Require a vanilla declaration kept by either side to survive the merge - #42
Open
TheValiantOne wants to merge 2 commits into
Open
Require a vanilla declaration kept by either side to survive the merge#42TheValiantOne wants to merge 2 commits into
TheValiantOne wants to merge 2 commits into
Conversation
ValidateWholeFileMergeOutput's lost-unit check exempted the one shape that does the most damage. Its condition was required = (inOld && inNew) || (inOld && !inBase) || (inNew && !inBase); so a unit present in vanilla, kept by one input and absent from the other was not required to survive - it could vanish from the merged output with the merge still reporting success. From text alone that shape is indistinguishable from the case that actually breaks games: a mod shipping a whole-file copy taken from an OLDER game build simply has no copy of declarations vanilla added since, and a three-way diff reads that absence as a deletion. Found the hard way on a live next-gen install. A pre-4.0 copy of r4Game.ws erased CR4Game.OnHDRChangedEvent - an engine-called event whose body is GetGuiManager().OnHDRChanged() - from the merged output. The merge reported success, the scripts compiled cleanly, and the game then rendered its menu background with no main menu at all, with nothing anywhere naming the cause. Two other mods in the same load order did the same thing: mapMenu.ws lost OnFiltersChanged / SetInitialFilters / m_fxSetInitialFilters, exploration.ws lost CheckVector / DoHorseKick / OnHorseKick plus five member variables. In every case the declaration was present in vanilla AND in the other contributing mod. The check now requires every unit present in either input to survive; only a name both sides dropped may still disappear (and such a name never reaches the loop, which iterates the two inputs' own keys). This deliberately reclassifies "one mod deletes a vanilla function, the other keeps it" from a silent deletion into a violation, reversing an existing test that asserted the old behavior. It is the same principle the function-level engine has always applied one layer down - "a deletion never silently overrides a surviving edit - losing code silently is unrecoverable if the deletion was wrong, keeping an unwanted edit is not" (Core's CLAUDE.md) - which the whole-file path simply wasn't enforcing. A violation also doesn't skip the file: it routes to the function-level rescue first, exactly like every other violation, and that engine's own edit-survives-competing-deletion policy usually keeps the unit. ValidateWholeFileMergeOutput takes optional oldDescription/newDescription so the message can name the mod that lacks the declaration - the single most useful thing to say, since the fix is almost always "that mod is built for an older game version". Optional, so existing callers and tests are unaffected; DiffPlexMergeEngine passes the descriptions it already has. 5 new tests (179 total), plus the reversed one rewritten with its rationale: the violation and its wording, a name dropped by both sides still allowed through, the stale mod named on whichever side it is, and a graceful message without descriptions. dotnet format clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FP8H6rBLCGPBFRSVsF3Kgw
These two files were swept in by an over-broad 'git add -A' while the working tree carried unrelated in-progress work. They are a real behavior change to what a Tools dashboard click does (adding parameters: ['merge','--overwrite']) and belong in their own reviewed change, not riding along with a merge-engine invariant.
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.
The gap
ValidateWholeFileMergeOutput's lost-unit check exempted the one shape that does the most damage:A unit present in vanilla, kept by one input and absent from the other evaluates to
(T&&F) || (T&&!T) || (F&&!T)= false. Not required to survive — so it could vanish, with the merge still reporting success.From text alone that shape is indistinguishable from the case that actually breaks games: a mod shipping a whole-file copy taken from an older game build simply has no copy of declarations vanilla added since, and a three-way diff reads that absence as a deletion.
Found the hard way
On a live next-gen install, a pre-4.0 copy of
r4Game.wserasedCR4Game.OnHDRChangedEvent— an engine-called event whose body isGetGuiManager().OnHDRChanged()— from the merged output.The merge reported success. The scripts compiled cleanly. The game then rendered its menu background with no main menu at all, and nothing anywhere named the cause. Two other mods in the same load order did the same thing:
game/r4Game.wsOnHDRChangedEventmodAlwaysFullExpgame/gui/menus/mapMenu.wsOnFiltersChanged,SetInitialFilters,m_fxSetInitialFiltersmodFastTravelFromAnywheregame/vehicles/horse/states/exploration.wsCheckVector,DoHorseKick,OnHorseKick+ 5 member varsmodFearlessRoachIn every case the declaration was present in vanilla and in the other contributing mod. All three mods are pre-next-gen whole-file copies.
The change
Every unit present in either input must now survive. Only a name both sides dropped may still disappear — and such a name never reaches the loop, which iterates the two inputs' own keys.
ValidateWholeFileMergeOutputalso takes optionaloldDescription/newDescriptionso the message can name the mod that lacks the declaration, since the fix is almost always "that mod is built for an older game version". Optional, so existing callers and tests are unaffected;DiffPlexMergeEnginepasses the descriptions it already has.The reviewable decision
This reverses an existing test,
ValidateWholeFileMergeOutput_AllowsALegitimateDeletionPropagating, which asserted the old behaviour. I've rewritten it under a new name with the rationale inline rather than quietly deleting it.The trade: a mod that deliberately deletes a whole vanilla function no longer gets that deletion propagated silently. I think that's clearly the right side, for two reasons:
CLAUDE.md). The whole-file path simply wasn't enforcing what the codebase already believed.Whole-function deletion by a mod is rare; silently losing engine-called vanilla code is catastrophic and near-impossible to diagnose from the symptom (it cost most of a day, and the invariant found all three files in one pass once written).
Verification
5 new tests, 179 total, all passing.
dotnet format whitespace --verify-no-changesclean.AI-assisted development
Produced by Claude Code (Opus 5), including the live diagnosis that motivated it. Per
CONTRIBUTING.md: the three real-world cases above were measured on a real 274-mod install, not inferred.