diff --git a/WitcherScriptMerger.Core/CLAUDE.md b/WitcherScriptMerger.Core/CLAUDE.md index 619cb2c..fc1b09b 100644 --- a/WitcherScriptMerger.Core/CLAUDE.md +++ b/WitcherScriptMerger.Core/CLAUDE.md @@ -523,6 +523,31 @@ defects the original design produced on a live install): against the real broken output preserved in `docs/bugs/artifacts/` (gate fails it on exactly the orphaned accessor the game rejected) with zero false positives across real vanilla `r4Player.ws`/`player.ws`/`actor.ws`/`baseEffect.ws`. +- **A vanilla declaration kept by either side must survive the whole-file merge** + (`ValidateWholeFileMergeOutput`'s lost-unit check). This extends the function-level + engine's own long-standing "a deletion never silently overrides a surviving edit" + principle (above) to the whole-file path, which did not previously enforce it: a unit + present in vanilla, kept by one input and absent from the other used to be allowed to + vanish, on the reading that it was "a legitimate deletion propagating". From text alone + that shape is **indistinguishable** from the case that actually causes damage — 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. + Observed live on a next-gen install: a pre-4.0 `r4Game.ws` erased + `CR4Game.OnHDRChangedEvent` (engine-called, calls `GetGuiManager().OnHDRChanged()`) from + the merged output; the merge reported success, the scripts compiled, and the game + rendered its menu background with **no main menu at all**. Two other mods in the same + load order did the same to `mapMenu.ws` (`OnFiltersChanged`, `SetInitialFilters`, + `m_fxSetInitialFilters`) and `exploration.ws` (`CheckVector`, `DoHorseKick`, + `OnHorseKick` plus five member variables) — in every case the declaration was present in + vanilla *and* in the other contributing mod. Since the two cases can't be told apart, + this errs toward the survivable one: the deliberate-deletion mod loses its deletion, + rather than an engine-called vanilla event disappearing with nothing saying so. A + violation doesn't skip the file — it routes to the function-level rescue first, like + every other violation, and that engine's own edit-survives-competing-deletion policy + usually keeps the unit. `ValidateWholeFileMergeOutput` takes optional + `oldDescription`/`newDescription` purely so 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". Only a name *both* sides dropped may still disappear. - A gap that still exists between two intact units is compared as before — whitespace-tolerant, deliberately NOT comment-stripped — producing a `Decisions` note when content differs; vanilla's gap text still wins at non-insertion slots. diff --git a/WitcherScriptMerger.Core/Tools/DiffPlexMergeEngine.cs b/WitcherScriptMerger.Core/Tools/DiffPlexMergeEngine.cs index 861a4e4..3a43b22 100644 --- a/WitcherScriptMerger.Core/Tools/DiffPlexMergeEngine.cs +++ b/WitcherScriptMerger.Core/Tools/DiffPlexMergeEngine.cs @@ -268,7 +268,8 @@ public MergeEngineResult MergeHeadless( // from the ORIGINAL inputs, sidestepping the corrupted output // entirely), then fall through to the conflict-marker sidecar. if (FunctionLevelMergeEngine.ValidateWholeFileMergeOutput( - baseText, oldText, newText, result.MergedText, outputPath, out var invariantViolation)) + baseText, oldText, newText, result.MergedText, outputPath, out var invariantViolation, + oldDescription ?? source1.Name, newDescription ?? source2.Name)) { // A prior attempt at this same conflict may have left a sidecar // marker file behind (see below) - if this attempt now auto-solves diff --git a/WitcherScriptMerger.Core/Tools/FunctionLevelMergeEngine.cs b/WitcherScriptMerger.Core/Tools/FunctionLevelMergeEngine.cs index bc47e44..bc35aef 100644 --- a/WitcherScriptMerger.Core/Tools/FunctionLevelMergeEngine.cs +++ b/WitcherScriptMerger.Core/Tools/FunctionLevelMergeEngine.cs @@ -702,8 +702,25 @@ int At(Dictionary counts, string name) => // Inputs that don't extract cleanly make the judgement impossible - returns // true (trust the merge, as before this guard existed) - EXCEPT when the inputs // extract and the OUTPUT doesn't, which is itself corruption. + // Names the side a violation message is talking about. Falls back to "one mod"/"the + // other mod" when the caller supplied no descriptions (tests, and any future caller + // that has none) - the message stays true, just less specific. + static string DescribeSide(bool isOldSide, string oldDescription, string newDescription) + { + var description = isOldSide ? oldDescription : newDescription; + if (!string.IsNullOrWhiteSpace(description)) + return description; + return isOldSide ? "one mod" : "the other mod"; + } + + // oldDescription/newDescription are optional purely so the violation text can name + // the mod that lacks a vanilla declaration - the single most useful thing to tell a + // user here, since the fix is almost always "that mod is built for an older game + // version". Optional rather than required so existing callers and tests are + // unaffected. public static bool ValidateWholeFileMergeOutput( - string baseText, string oldText, string newText, string mergedText, string outputPath, out string violation) + string baseText, string oldText, string newText, string mergedText, string outputPath, out string violation, + string oldDescription = null, string newDescription = null) { violation = null; if (!FileIndex.ModFile.IsScript(outputPath)) @@ -764,17 +781,50 @@ public static bool ValidateWholeFileMergeOutput( } } + // Every unit present in EITHER input must survive. Only a name both sides + // dropped may legitimately disappear - and such a name never reaches this loop, + // since it iterates the two inputs' own keys. + // + // The vanilla-retention case (in base, kept by one side, absent from the other) + // used to be excluded here, and that exclusion caused real, silent damage. A mod + // shipping a whole-file copy taken from an OLDER game build simply doesn't + // contain declarations the current vanilla added since - which a three-way diff + // cannot tell apart from "this mod deleted it", so the deletion won and the + // merged file shipped without it. Observed live on a next-gen install: a + // pre-4.0 copy of r4Game.ws erased `CR4Game.OnHDRChangedEvent` (an + // engine-called event that calls GetGuiManager().OnHDRChanged()) from the merged + // output; the merge reported success, the scripts compiled, and the game then + // rendered its menu background with no main menu at all. Two more mods did the + // same thing in the same load order - 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. + // + // Treating this as a violation deliberately reclassifies a genuine + // "one mod deletes a vanilla function, the other keeps it" case as a conflict + // rather than a silent deletion. That is the right trade: whole-function + // deletion by a mod is rare, silently losing engine-called vanilla code is + // catastrophic and near-impossible to diagnose from the symptom, and a violation + // here doesn't skip the file outright - it routes to the function-level rescue + // first, exactly like every other violation. foreach (var name in oldCounts.Keys.Concat(newCounts.Keys).Distinct()) { var inOld = At(oldCounts, name) > 0; var inNew = At(newCounts, name) > 0; var inBase = At(baseCounts, name) > 0; - var required = (inOld && inNew) || (inOld && !inBase) || (inNew && !inBase); - if (required && At(mergedCounts, name) == 0) - { - violation = $"'{name}' is present in {(inOld && inNew ? "both inputs" : "an input that inserted it")} but missing from the merged output (lost)"; - return false; - } + if (At(mergedCounts, name) != 0) + continue; + + if (inOld && inNew) + violation = $"'{name}' is present in both inputs but missing from the merged output (lost)"; + else if (!inBase) + violation = $"'{name}' is present in an input that inserted it but missing from the merged output (lost)"; + else + violation = + $"'{name}' is declared in the vanilla file and kept by {DescribeSide(inOld, oldDescription, newDescription)}, " + + $"but is missing from the merged output (lost) - {DescribeSide(!inOld, oldDescription, newDescription)} has no copy of it, " + + "which usually means that mod ships a whole-file copy taken from an older game build"; + return false; } // Line-level: DiffPlex's silent duplication can strike INSIDE a function diff --git a/WitcherScriptMerger.Tests/CLAUDE.md b/WitcherScriptMerger.Tests/CLAUDE.md index bd1bd0f..703e410 100644 --- a/WitcherScriptMerger.Tests/CLAUDE.md +++ b/WitcherScriptMerger.Tests/CLAUDE.md @@ -32,7 +32,11 @@ does. most-distinct-from-vanilla tiebreak (including its deterministic tie-break and a scaled-down `DiffAlgorithmException` case), edit-survives-competing-deletion, insertion reconciliation (including the same-name-different-body decline case), and gap-comment - detection. + detection. Also `ValidateWholeFileMergeOutput`'s lost-unit check, including the + deliberately reversed expectation for a vanilla declaration kept by one side and + absent from the other (now a violation, not "a legitimate deletion propagating" - + see Core's `CLAUDE.md`), that a name dropped by *both* sides is still allowed to go, + and that the violation message names the stale mod on whichever side it is. - `Inventory/FileMergerTests.cs` — `FileMerger.IsVanillaDlcBundleFolder`: known vanilla DLC-folder names, case-insensitivity, and non-matches (including anchoring) — see Core's `CLAUDE.md`'s "Vortex-fork parity fixes" section. Also covers the two-arg diff --git a/WitcherScriptMerger.Tests/Tools/FunctionLevelMergeEngineTests.cs b/WitcherScriptMerger.Tests/Tools/FunctionLevelMergeEngineTests.cs index 4a9bc10..1548ac3 100644 --- a/WitcherScriptMerger.Tests/Tools/FunctionLevelMergeEngineTests.cs +++ b/WitcherScriptMerger.Tests/Tools/FunctionLevelMergeEngineTests.cs @@ -554,17 +554,101 @@ public void ValidateWholeFileMergeOutput_DetectsSilentLossOfAnInsertion() Assert.Contains("lost", violation); } - [Fact] - public void ValidateWholeFileMergeOutput_AllowsALegitimateDeletionPropagating() + // DELIBERATE REVERSAL of this test's previous expectation, which asserted that a + // vanilla declaration present in one input and absent from the other could legitimately + // vanish from the merged output ("a legitimate deletion propagating"). + // + // That shape is indistinguishable, from text alone, from the failure that motivated this + // change: a mod shipping a whole-file copy taken from an OLDER game build has no copy of + // declarations vanilla added since, and a three-way diff reads that absence as a + // deletion. Observed live on a next-gen install - a pre-4.0 r4Game.ws erased + // CR4Game.OnHDRChangedEvent (engine-called, calls GetGuiManager().OnHDRChanged()) from + // the merged output; the merge reported success, the scripts compiled, and the game + // rendered its menu background with no main menu. Two other mods in the same load order + // did the same to mapMenu.ws and exploration.ws. + // + // Since the two cases cannot be told apart, this errs toward the survivable one. A + // violation does not skip the file - it routes to the function-level rescue first, which + // re-merges per unit and has its own edit-survives-competing-deletion policy. The cost is + // that a deliberate whole-function deletion no longer propagates silently; the benefit is + // that engine-called vanilla code can no longer disappear with nothing saying so. + [Fact] + public void ValidateWholeFileMergeOutput_VanillaDeclarationKeptByOneSideButLost_IsAViolation() { var baseText = Fn("A", "\ta();\r\n") + "\r\n" + Fn("B", "\tb();\r\n"); var oldText = baseText; var newText = Fn("A", "\ta();\r\n"); var mergedText = Fn("A", "\ta();\r\n"); + Assert.False(FunctionLevelMergeEngine.ValidateWholeFileMergeOutput(baseText, oldText, newText, mergedText, "x.ws", out var violation)); + Assert.Contains("lost", violation); + Assert.Contains("older game build", violation); + } + + // The one loss that stays legitimate: neither input has it any more, so nothing is being + // discarded. Such a name never even reaches the check, since it iterates the two inputs' + // own keys - this pins that down. + [Fact] + public void ValidateWholeFileMergeOutput_VanillaDeclarationDroppedByBothSides_IsAllowed() + { + var baseText = Fn("A", "\ta();\r\n") + "\r\n" + Fn("B", "\tb();\r\n"); + var oldText = Fn("A", "\ta();\r\n"); + var newText = Fn("A", "\ta();\r\n"); + var mergedText = Fn("A", "\ta();\r\n"); + Assert.True(FunctionLevelMergeEngine.ValidateWholeFileMergeOutput(baseText, oldText, newText, mergedText, "x.ws", out _)); } + // The most useful thing this message can carry is WHICH mod lacks the declaration, + // because the fix is almost always "that mod is built for an older game version". + [Fact] + public void ValidateWholeFileMergeOutput_NamesTheModThatLacksTheVanillaDeclaration() + { + var baseText = Fn("A", "\ta();\r\n") + "\r\n" + Fn("B", "\tb();\r\n"); + var oldText = baseText; + var newText = Fn("A", "\ta();\r\n"); + var mergedText = Fn("A", "\ta();\r\n"); + + Assert.False(FunctionLevelMergeEngine.ValidateWholeFileMergeOutput( + baseText, oldText, newText, mergedText, "x.ws", out var violation, "modKeepsIt", "modStale")); + + Assert.Contains("kept by modKeepsIt", violation); + Assert.Contains("modStale has no copy of it", violation); + } + + // Same case with the stale side reversed, so the message cannot be passing by hardcoding + // one side. + [Fact] + public void ValidateWholeFileMergeOutput_NamesTheStaleModWhicheverSideItIs() + { + var baseText = Fn("A", "\ta();\r\n") + "\r\n" + Fn("B", "\tb();\r\n"); + var oldText = Fn("A", "\ta();\r\n"); + var newText = baseText; + var mergedText = Fn("A", "\ta();\r\n"); + + Assert.False(FunctionLevelMergeEngine.ValidateWholeFileMergeOutput( + baseText, oldText, newText, mergedText, "x.ws", out var violation, "modStale", "modKeepsIt")); + + Assert.Contains("kept by modKeepsIt", violation); + Assert.Contains("modStale has no copy of it", violation); + } + + // Descriptions are optional - without them the message must still be true, just less + // specific. + [Fact] + public void ValidateWholeFileMergeOutput_WithoutDescriptions_StillDescribesBothSides() + { + var baseText = Fn("A", "\ta();\r\n") + "\r\n" + Fn("B", "\tb();\r\n"); + var oldText = baseText; + var newText = Fn("A", "\ta();\r\n"); + var mergedText = Fn("A", "\ta();\r\n"); + + Assert.False(FunctionLevelMergeEngine.ValidateWholeFileMergeOutput(baseText, oldText, newText, mergedText, "x.ws", out var violation)); + + Assert.Contains("kept by one mod", violation); + Assert.Contains("the other mod has no copy of it", violation); + } + [Fact] public void ValidateWholeFileMergeOutput_NonScriptFiles_AlwaysTrusted() {