From f106a1f857fde69d246c2ce76710c34f51bea742 Mon Sep 17 00:00:00 2001 From: Chris Knight Date: Fri, 21 Aug 2026 23:46:31 -0400 Subject: [PATCH 1/2] Let a mod ranking decide whose version of a function wins Adds an opt-in, user-supplied mod ranking that resolves the whole-function tiebreak. Until now that tiebreak was "most distinct from vanilla wins" - a guess at which side did more work, with no way for a user to say "I want this mod's version of this function." Deliberately a different lever from the existing per-file order override (FileMerger.ResolveMergeOrder): that sets the chain ORDER and must name every mod for the file it covers. This one reorders nothing. It only answers "whose code survives" at the exact point FunctionLevelMergeEngine was already going to pick a whole side - never overriding a clean, genuinely-merged result. Partial by design: rank only the mods you care about. A pair where neither side is ranked returns PreferredSide.None and behaves exactly as before, so adding a ranking can never change the outcome of a conflict it doesn't mention. A ranked mod beats an unranked one. The accumulated side of a chain step carries every mod merged into it so far, so it takes the BEST rank among them - without that, a highly-ranked mod would stop winning the moment one more mod merged on top of it, which is the opposite of what ranking it means. Configured through the existing order file's reserved "*" entry, alongside its ordinary per-path entries. That reuses all the existing plumbing at once - both hosts' --order-file, the MCP merge_conflicts orderOverrides argument, and the Vortex extension - with no new config surface. "*" is safe as a sentinel because it's a reserved character in Windows paths, so it can never collide with a real relative path; MergeConflictsHeadless strips it before ResolveMergeOrder can ever see it as a path override. Every ranking-driven decision is recorded in the same Decisions audit trail as every other non-mechanical choice, naming the ranking as the reason. 18 new tests (193 total): rank comparison in both directions, ranked-beats- unranked, the accumulated side taking its best rank, case-insensitivity, no-opinion cases (no ranking, neither side ranked, same mod both sides), and the order-file plumbing (extraction, blank trimming, and that the reserved key never reaches ResolveMergeOrder). dotnet format clean. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FP8H6rBLCGPBFRSVsF3Kgw --- .../Inventory/FileMerger.cs | 22 ++- .../LoadOrder/ModPriority.cs | 129 +++++++++++++ .../Tools/DiffPlexMergeEngine.cs | 14 +- .../Tools/FunctionLevelMergeEngine.cs | 34 +++- .../LoadOrder/ModPriorityTests.cs | 182 ++++++++++++++++++ 5 files changed, 368 insertions(+), 13 deletions(-) create mode 100644 WitcherScriptMerger.Core/LoadOrder/ModPriority.cs create mode 100644 WitcherScriptMerger.Tests/LoadOrder/ModPriorityTests.cs diff --git a/WitcherScriptMerger.Core/Inventory/FileMerger.cs b/WitcherScriptMerger.Core/Inventory/FileMerger.cs index ad3ee66..6774301 100644 --- a/WitcherScriptMerger.Core/Inventory/FileMerger.cs +++ b/WitcherScriptMerger.Core/Inventory/FileMerger.cs @@ -151,6 +151,11 @@ public class MergeReportData // only the entries for paths that actually made it into summary.Merged. Dictionary> _functionLevelDecisionsByPath = new Dictionary>(StringComparer.OrdinalIgnoreCase); + // The global mod ranking for this run, pulled out of the order file's reserved + // "*" entry (see LoadOrder/ModPriority). Null when none was supplied, which is + // the normal case - every tiebreak then behaves exactly as it did before. + string[] _modPriority; + // Anchored at BOTH ends ("^...$") - see IsVanillaDlcBundleFolder's own comment // below for why this matters: it's matched against just the extracted folder-name // segment, not the full path, so a full-string match is required, not merely a @@ -485,6 +490,12 @@ public HeadlessMergeSummary MergeConflictsHeadless( bool dryRun = false, bool overwrite = false) { + // The ranking rides in the same order-file payload as the per-file overrides, so + // it reaches every host and the MCP tool without new plumbing. Split out here, + // once, so ResolveMergeOrder only ever sees real relative-path entries. + _modPriority = ModPriority.ExtractRanking(orderOverrides); + orderOverrides = ModPriority.WithoutRankingEntry(orderOverrides); + var summary = new HeadlessMergeSummary(); foreach (var conflict in conflicts.Where(c => @@ -632,7 +643,8 @@ bool MergeFlatConflictHeadless(ModFile conflict, Merge merge, string mergedModNa var hash = conflict.Mods.First(h => h.Name.EqualsIgnoreCase(orderedNames[i])); var source2 = MergeSource.FromFlatFile(new FileInfo(conflict.GetModFile(orderedNames[i])), hash); - var mergedFile = MergeTextHeadless(merge, source1, source2, dryRun, DescribeAccumulated(orderedNames.Take(i)), orderedNames[i]); + var mergedFile = MergeTextHeadless(merge, source1, source2, dryRun, DescribeAccumulated(orderedNames.Take(i)), orderedNames[i], + ModPriority.Resolve(_modPriority, orderedNames.Take(i), orderedNames[i])); if (mergedFile == null) return false; source1 = MergeSource.FromFlatFile(mergedFile, null); @@ -689,7 +701,8 @@ bool MergeBundleConflictHeadless(ModFile conflict, Merge merge, string[] ordered if (!GetUnpackedFiles(conflict.RelativePath, ref source1, ref source2)) return false; - var mergedFile = MergeTextHeadless(merge, source1, source2, dryRun, DescribeAccumulated(orderedNames.Take(i)), orderedNames[i]); + var mergedFile = MergeTextHeadless(merge, source1, source2, dryRun, DescribeAccumulated(orderedNames.Take(i)), orderedNames[i], + ModPriority.Resolve(_modPriority, orderedNames.Take(i), orderedNames[i])); if (mergedFile == null) return false; source1 = MergeSource.FromFlatFile(mergedFile, null); @@ -773,7 +786,8 @@ string[] ResolveMergeOrder(ModFile conflict, string mergedModName, IReadOnlyDict .ToArray(); } - FileInfo MergeTextHeadless(Merge merge, MergeSource source1, MergeSource source2, bool dryRun, string oldDescription = null, string newDescription = null) + FileInfo MergeTextHeadless(Merge merge, MergeSource source1, MergeSource source2, bool dryRun, string oldDescription = null, string newDescription = null, + PreferredSide preferredSide = PreferredSide.None) { ProgressInfo.CurrentAction = $"Merging {source1.Name} && {source2.Name}"; @@ -786,7 +800,7 @@ FileInfo MergeTextHeadless(Merge merge, MergeSource source1, MergeSource source2 // review before it shipped (see docs/decisions/kdiff3-retirement.md). var result = _mergeEngine.MergeHeadless( source1, source2, _vanillaFile, _outputPath, openConflictMarkers: !dryRun, - oldDescription: oldDescription, newDescription: newDescription); + oldDescription: oldDescription, newDescription: newDescription, preferredSide: preferredSide); if (_mergeEngine.LastFunctionLevelDecisions.Count > 0) { diff --git a/WitcherScriptMerger.Core/LoadOrder/ModPriority.cs b/WitcherScriptMerger.Core/LoadOrder/ModPriority.cs new file mode 100644 index 0000000..6c1d2f0 --- /dev/null +++ b/WitcherScriptMerger.Core/LoadOrder/ModPriority.cs @@ -0,0 +1,129 @@ +using System; +using System.Collections.Generic; +using System.Linq; + +namespace WitcherScriptMerger.LoadOrder +{ + // Which side of a pairwise merge step a mod ranking prefers. `None` means the ranking + // has no opinion here and the engine's existing behavior applies unchanged - the normal + // case, since a ranking is opt-in and may be partial. + public enum PreferredSide + { + None, + Old, + New, + } + + // A user-supplied ranking of mods, used to decide whose version of a function survives + // when two mods edit the same function differently. + // + // This is a different lever from the existing per-file order override + // (FileMerger.ResolveMergeOrder): that one sets the *chain order* mods are merged in, + // and must name every mod for the file it covers. This one doesn't reorder anything - + // it answers "whose code wins" at the point FunctionLevelMergeEngine would otherwise + // fall back to its most-distinct-from-vanilla tiebreak, which is order-independent and + // has no notion of the user preferring one mod over another. + // + // Deliberately partial: rank only the mods you care about. A pair where neither side is + // ranked returns None and behaves exactly as before, so adding a ranking can never + // change the outcome of a conflict it doesn't mention. + public static class ModPriority + { + // Reserved key an order file uses for the global ranking, alongside its ordinary + // "": [mods...] entries. Safe as a sentinel because `*` is a + // reserved character in Windows file and directory names, so it can never collide + // with a real conflict's relative path. + public const string OrderFileKey = "*"; + + // Lower index = higher priority, so rank 0 beats rank 1. An unranked mod sits below + // every ranked one. + const int Unranked = int.MaxValue; + + static int RankOf(IReadOnlyList ranking, string modName) + { + if (ranking == null || string.IsNullOrWhiteSpace(modName)) + return Unranked; + + for (var i = 0; i < ranking.Count; ++i) + { + if (ranking[i] != null && ranking[i].EqualsIgnoreCase(modName)) + return i; + } + return Unranked; + } + + /// + /// Decides which side of one pairwise chain step the ranking prefers. + /// + /// Mod names, highest priority first. Null/empty = no opinion. + /// + /// Every mod already accumulated into the old side. A chain step's old side is the + /// merge of everything before it, so it carries several mods at once; its rank is the + /// BEST rank among them. Without that, a highly-ranked mod would stop winning as soon + /// as one more mod merged on top of it, which is the opposite of what ranking it + /// means. + /// + /// The single mod being merged in at this step. + public static PreferredSide Resolve(IReadOnlyList ranking, IEnumerable oldModNames, string newModName) + { + if (ranking == null || ranking.Count == 0) + return PreferredSide.None; + + var oldRank = Unranked; + if (oldModNames != null) + { + foreach (var name in oldModNames) + oldRank = Math.Min(oldRank, RankOf(ranking, name)); + } + + var newRank = RankOf(ranking, newModName); + + // Neither side is mentioned - stay out of it entirely. + if (oldRank == Unranked && newRank == Unranked) + return PreferredSide.None; + + if (oldRank < newRank) + return PreferredSide.Old; + if (newRank < oldRank) + return PreferredSide.New; + + // Equal ranks means the same mod appears on both sides, which the chain + // shouldn't produce. No opinion rather than an arbitrary pick. + return PreferredSide.None; + } + + /// + /// Pulls the global ranking out of an order file's entries, or null if it has none. + /// The reserved key is removed from the caller's view of the dictionary by + /// so it can never be mistaken for a path override. + /// + public static string[] ExtractRanking(IReadOnlyDictionary orderOverrides) + { + if (orderOverrides == null || !orderOverrides.TryGetValue(OrderFileKey, out var ranking) || ranking == null) + return null; + + // Blank entries are dropped rather than rejected: a ranking is advisory, and a + // stray empty string in a hand-edited file shouldn't fail a whole merge run. + var cleaned = ranking + .Where(name => !string.IsNullOrWhiteSpace(name)) + .Select(name => name.Trim()) + .ToArray(); + + return cleaned.Length == 0 ? null : cleaned; + } + + /// + /// The order-file entries with the reserved ranking key removed, so + /// ResolveMergeOrder only ever sees real relative-path overrides. + /// + public static IReadOnlyDictionary WithoutRankingEntry(IReadOnlyDictionary orderOverrides) + { + if (orderOverrides == null || !orderOverrides.ContainsKey(OrderFileKey)) + return orderOverrides; + + return orderOverrides + .Where(kvp => !string.Equals(kvp.Key, OrderFileKey, StringComparison.Ordinal)) + .ToDictionary(kvp => kvp.Key, kvp => kvp.Value); + } + } +} diff --git a/WitcherScriptMerger.Core/Tools/DiffPlexMergeEngine.cs b/WitcherScriptMerger.Core/Tools/DiffPlexMergeEngine.cs index 861a4e4..17eb91f 100644 --- a/WitcherScriptMerger.Core/Tools/DiffPlexMergeEngine.cs +++ b/WitcherScriptMerger.Core/Tools/DiffPlexMergeEngine.cs @@ -10,6 +10,7 @@ using DiffPlex.Model; using WitcherScriptMerger.FileIndex; using WitcherScriptMerger.Inventory; +using WitcherScriptMerger.LoadOrder; namespace WitcherScriptMerger.Tools { @@ -159,7 +160,8 @@ public MergeEngineResult MergeHeadless( string outputPath, bool openConflictMarkers = true, string oldDescription = null, - string newDescription = null) + string newDescription = null, + PreferredSide preferredSide = PreferredSide.None) { LastFunctionLevelDecisions = Array.Empty(); @@ -233,7 +235,7 @@ public MergeEngineResult MergeHeadless( // merge even when the whole-file 3-way diff hits this bug. Only // attempted for .ws files - the extractor is WitcherScript-specific and // has no notion of XML structure. - if (TryFunctionLevelRescue(baseText, oldText, newText, source1, source2, outputPath, oldDescription, newDescription, openConflictMarkers)) + if (TryFunctionLevelRescue(baseText, oldText, newText, source1, source2, outputPath, oldDescription, newDescription, openConflictMarkers, preferredSide)) return MergeEngineResult.AutoSolved; // DiffPlex's own diff algorithm produced output it isn't safe to trust @@ -287,7 +289,7 @@ public MergeEngineResult MergeHeadless( "DiffPlex ThreeWayDiffer bug (see CLAUDE.md). Falling back to the function-level merge.", "Merge Output Rejected", NotifyButtons.OK, DialogIcon.Warning); - if (TryFunctionLevelRescue(baseText, oldText, newText, source1, source2, outputPath, oldDescription, newDescription, openConflictMarkers)) + if (TryFunctionLevelRescue(baseText, oldText, newText, source1, source2, outputPath, oldDescription, newDescription, openConflictMarkers, preferredSide)) return MergeEngineResult.AutoSolved; // Same policy as the DiffAlgorithmException branch above, for the same @@ -308,7 +310,7 @@ public MergeEngineResult MergeHeadless( // FunctionLevelMergeEngine's own comment for why this is a fallback that // only ever activates where the whole-file merge has already failed, never // a parallel code path for merges that would have succeeded anyway. - if (TryFunctionLevelRescue(baseText, oldText, newText, source1, source2, outputPath, oldDescription, newDescription, openConflictMarkers)) + if (TryFunctionLevelRescue(baseText, oldText, newText, source1, source2, outputPath, oldDescription, newDescription, openConflictMarkers, preferredSide)) return MergeEngineResult.AutoSolved; // Never write conflict markers to outputPath itself: FileMerger's headless @@ -455,7 +457,7 @@ static void DeleteIfExists(string path) bool TryFunctionLevelRescue( string baseText, string oldText, string newText, FileMerger.MergeSource source1, FileMerger.MergeSource source2, string outputPath, - string oldDescription, string newDescription, bool openConflictMarkers) + string oldDescription, string newDescription, bool openConflictMarkers, PreferredSide preferredSide) { // ModFile.IsScript, not a locally reinvented extension check - the same // vocabulary every other file-category dispatch in Core uses for this exact @@ -470,7 +472,7 @@ bool TryFunctionLevelRescue( result = FunctionLevelMergeEngine.TryMerge( baseText, oldText, newText, source1.Name, source2.Name, - oldDescription ?? source1.Name, newDescription ?? source2.Name); + oldDescription ?? source1.Name, newDescription ?? source2.Name, preferredSide); } catch (ScriptUnitExtractor.ExtractionException) { diff --git a/WitcherScriptMerger.Core/Tools/FunctionLevelMergeEngine.cs b/WitcherScriptMerger.Core/Tools/FunctionLevelMergeEngine.cs index bc47e44..0588816 100644 --- a/WitcherScriptMerger.Core/Tools/FunctionLevelMergeEngine.cs +++ b/WitcherScriptMerger.Core/Tools/FunctionLevelMergeEngine.cs @@ -4,6 +4,7 @@ using System.Text; using System.Text.RegularExpressions; using DiffPlex; +using WitcherScriptMerger.LoadOrder; namespace WitcherScriptMerger.Tools { @@ -53,10 +54,15 @@ public FunctionLevelMergeResult(bool applied, string mergedText, IReadOnlyList decisions) + List decisions, PreferredSide preferredSide) { var baseText = baseUnit.FullText; @@ -357,6 +363,28 @@ static string ResolveUnit( "tiebreak below instead of the spliced result."); } + // A user-supplied mod ranking outranks the distinctness heuristic. Distinctness + // is a guess at which side "did more" and has no way to know the user simply + // wants a particular mod's version of a function to survive; a ranking is that + // user telling us directly. Only consulted here, at the point the engine was + // already going to pick a whole side - never anywhere that would discard a + // clean, genuinely-merged result. + if (preferredSide == PreferredSide.New) + { + decisions.Add( + $"{baseUnit.DescribeKind()} {baseUnit.Name}: kept {newDescription}'s version because the configured " + + $"mod ranking prefers it over {oldDescription}, discarding {oldDescription}'s conflicting change here."); + return newText; + } + + if (preferredSide == PreferredSide.Old) + { + decisions.Add( + $"{baseUnit.DescribeKind()} {baseUnit.Name}: kept {oldDescription}'s version because the configured " + + $"mod ranking prefers it over {newDescription}, discarding {newDescription}'s conflicting change here."); + return oldText; + } + var oldDistinctness = ComputeDistinctness(baseText, oldText); var newDistinctness = ComputeDistinctness(baseText, newText); diff --git a/WitcherScriptMerger.Tests/LoadOrder/ModPriorityTests.cs b/WitcherScriptMerger.Tests/LoadOrder/ModPriorityTests.cs new file mode 100644 index 0000000..0c104d8 --- /dev/null +++ b/WitcherScriptMerger.Tests/LoadOrder/ModPriorityTests.cs @@ -0,0 +1,182 @@ +using System.Collections.Generic; +using WitcherScriptMerger.LoadOrder; +using Xunit; + +namespace WitcherScriptMerger.Tests.LoadOrder +{ + // Coverage for ModPriority - the user-supplied mod ranking that decides whose version of + // a function survives when two mods edit it differently, at the point + // FunctionLevelMergeEngine would otherwise fall back to its most-distinct-from-vanilla + // tiebreak. + // + // Deliberately a different lever from FileMerger.ResolveMergeOrder's per-file order + // override: that sets the chain ORDER and must name every mod for the file it covers. + // This one reorders nothing and is partial by design - a pair where neither side is + // ranked must behave exactly as it did before the ranking existed. + // + // Pure static over plain strings: no filesystem, no AppState - see + // WitcherScriptMerger.Tests/CLAUDE.md's "AppState.Settings-safety constraints". + public class ModPriorityTests + { + static readonly string[] Ranking = { "modWinner", "modMiddle", "modLoser" }; + + [Fact] + public void Resolve_NoRanking_HasNoOpinion() + { + Assert.Equal(PreferredSide.None, ModPriority.Resolve(null, new[] { "modA" }, "modB")); + Assert.Equal(PreferredSide.None, ModPriority.Resolve(new string[0], new[] { "modA" }, "modB")); + } + + // The whole point of "partial rankings allowed": adding a ranking must not change the + // outcome of a conflict it doesn't mention. + [Fact] + public void Resolve_NeitherSideRanked_HasNoOpinion() + { + Assert.Equal(PreferredSide.None, ModPriority.Resolve(Ranking, new[] { "modUnknownA" }, "modUnknownB")); + } + + [Fact] + public void Resolve_HigherRankedOnNewSide_PrefersNew() + { + Assert.Equal(PreferredSide.New, ModPriority.Resolve(Ranking, new[] { "modLoser" }, "modWinner")); + } + + [Fact] + public void Resolve_HigherRankedOnOldSide_PrefersOld() + { + Assert.Equal(PreferredSide.Old, ModPriority.Resolve(Ranking, new[] { "modWinner" }, "modLoser")); + } + + // A ranked mod beats an unranked one - that's what ranking it means. + [Fact] + public void Resolve_RankedBeatsUnranked_EitherSide() + { + Assert.Equal(PreferredSide.New, ModPriority.Resolve(Ranking, new[] { "modUnranked" }, "modMiddle")); + Assert.Equal(PreferredSide.Old, ModPriority.Resolve(Ranking, new[] { "modMiddle" }, "modUnranked")); + } + + // The accumulated side carries every mod merged into it so far, so it takes the BEST + // rank among them. Without this a highly-ranked mod would stop winning the moment one + // more mod merged on top of it - the opposite of what ranking it means. + [Fact] + public void Resolve_AccumulatedSideTakesItsBestRank() + { + var accumulated = new[] { "modUnranked", "modWinner", "modLoser" }; + + Assert.Equal(PreferredSide.Old, ModPriority.Resolve(Ranking, accumulated, "modMiddle")); + } + + [Fact] + public void Resolve_AccumulatedSideAllUnranked_LosesToARankedNewMod() + { + var accumulated = new[] { "modUnrankedA", "modUnrankedB" }; + + Assert.Equal(PreferredSide.New, ModPriority.Resolve(Ranking, accumulated, "modLoser")); + } + + [Fact] + public void Resolve_IsCaseInsensitive() + { + Assert.Equal(PreferredSide.New, ModPriority.Resolve(Ranking, new[] { "MODLOSER" }, "modwinner")); + } + + [Fact] + public void Resolve_SameModOnBothSides_HasNoOpinion() + { + Assert.Equal(PreferredSide.None, ModPriority.Resolve(Ranking, new[] { "modWinner" }, "modWinner")); + } + + [Fact] + public void Resolve_EmptyOrNullAccumulatedSide_StillRanksTheNewMod() + { + Assert.Equal(PreferredSide.New, ModPriority.Resolve(Ranking, null, "modWinner")); + Assert.Equal(PreferredSide.New, ModPriority.Resolve(Ranking, new string[0], "modWinner")); + } + + #region Order-file plumbing + + [Fact] + public void ExtractRanking_NoReservedEntry_ReturnsNull() + { + var overrides = new Dictionary { [@"game\actor.ws"] = new[] { "modA", "modB" } }; + + Assert.Null(ModPriority.ExtractRanking(overrides)); + } + + [Fact] + public void ExtractRanking_ReturnsTheReservedEntry() + { + var overrides = new Dictionary + { + [ModPriority.OrderFileKey] = new[] { "modWinner", "modLoser" }, + [@"game\actor.ws"] = new[] { "modA", "modB" }, + }; + + Assert.Equal(new[] { "modWinner", "modLoser" }, ModPriority.ExtractRanking(overrides)); + } + + // A ranking is advisory - a stray blank in a hand-edited order file trims away + // rather than failing a whole merge run. + [Fact] + public void ExtractRanking_DropsBlankEntriesAndTrims() + { + var overrides = new Dictionary + { + [ModPriority.OrderFileKey] = new[] { " modWinner ", "", " ", "modLoser" }, + }; + + Assert.Equal(new[] { "modWinner", "modLoser" }, ModPriority.ExtractRanking(overrides)); + } + + [Fact] + public void ExtractRanking_EmptyRanking_ReturnsNull() + { + var overrides = new Dictionary { [ModPriority.OrderFileKey] = new string[0] }; + + Assert.Null(ModPriority.ExtractRanking(overrides)); + } + + [Fact] + public void ExtractRanking_NullRanking_ReturnsNull() + { + var overrides = new Dictionary { [ModPriority.OrderFileKey] = null }; + + Assert.Null(ModPriority.ExtractRanking(overrides)); + } + + // ResolveMergeOrder looks entries up by relative path; the reserved key must never + // reach it, or a file literally named "*" would be the only thing standing between a + // ranking and a bogus "unknown mod" failure. + [Fact] + public void WithoutRankingEntry_RemovesOnlyTheReservedKey() + { + var overrides = new Dictionary + { + [ModPriority.OrderFileKey] = new[] { "modWinner" }, + [@"game\actor.ws"] = new[] { "modA", "modB" }, + }; + + var stripped = ModPriority.WithoutRankingEntry(overrides); + + Assert.False(stripped.ContainsKey(ModPriority.OrderFileKey)); + Assert.Equal(new[] { "modA", "modB" }, stripped[@"game\actor.ws"]); + Assert.Single(stripped); + } + + [Fact] + public void WithoutRankingEntry_NoReservedKey_ReturnsInputUnchanged() + { + var overrides = new Dictionary { [@"game\actor.ws"] = new[] { "modA", "modB" } }; + + Assert.Same(overrides, ModPriority.WithoutRankingEntry(overrides)); + } + + [Fact] + public void WithoutRankingEntry_Null_IsTolerated() + { + Assert.Null(ModPriority.WithoutRankingEntry(null)); + } + + #endregion + } +} From eadaf1e0a22fbfe6fff4dc58a932b92878f744be Mon Sep 17 00:00:00 2001 From: Chris Knight Date: Sat, 22 Aug 2026 00:07:51 -0400 Subject: [PATCH 2/2] Preserve the source key comparer in WithoutRankingEntry A plain ToDictionary rebuilds with the default ordinal comparer, silently undoing the StringComparer.OrdinalIgnoreCase that WsmMcpTools.MergeConflicts deliberately installs (alongside separator normalization) so a differently-cased but otherwise correct path key isn't ignored. Dropping it turned those keys back into silent no-ops - but only for callers that supply a ranking, so the bug would have stayed hidden until the feature was actually used. Now reads the source dictionary's own comparer and reuses it, falling back to the default only when it can't be read - so it neither drops a deliberately-installed comparer nor imposes one on the CLI hosts, which build their order dictionary with the default and must stay exactly as case-sensitive as before. Found by an independent review pass over this branch, not by the tests - nothing exercised the interaction. 2 new tests (195 total) cover both directions. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FP8H6rBLCGPBFRSVsF3Kgw --- .../LoadOrder/ModPriority.cs | 12 +++++- .../LoadOrder/ModPriorityTests.cs | 40 ++++++++++++++++++- 2 files changed, 50 insertions(+), 2 deletions(-) diff --git a/WitcherScriptMerger.Core/LoadOrder/ModPriority.cs b/WitcherScriptMerger.Core/LoadOrder/ModPriority.cs index 6c1d2f0..642a4ca 100644 --- a/WitcherScriptMerger.Core/LoadOrder/ModPriority.cs +++ b/WitcherScriptMerger.Core/LoadOrder/ModPriority.cs @@ -121,9 +121,19 @@ public static IReadOnlyDictionary WithoutRankingEntry(IReadOnl if (orderOverrides == null || !orderOverrides.ContainsKey(OrderFileKey)) return orderOverrides; + // Preserve the source dictionary's key comparer. A plain ToDictionary would + // rebuild with the default ordinal one and silently undo a deliberately-installed + // case-insensitive comparer - WsmMcpTools.MergeConflicts builds exactly that + // (StringComparer.OrdinalIgnoreCase, alongside separator normalization) so a + // differently-cased but otherwise-correct path key isn't ignored. Dropping it here + // would turn those keys back into silent no-ops, but only for callers that supply + // a ranking - a bug that hides until the feature is used. + var comparer = (orderOverrides as Dictionary)?.Comparer + ?? EqualityComparer.Default; + return orderOverrides .Where(kvp => !string.Equals(kvp.Key, OrderFileKey, StringComparison.Ordinal)) - .ToDictionary(kvp => kvp.Key, kvp => kvp.Value); + .ToDictionary(kvp => kvp.Key, kvp => kvp.Value, comparer); } } } diff --git a/WitcherScriptMerger.Tests/LoadOrder/ModPriorityTests.cs b/WitcherScriptMerger.Tests/LoadOrder/ModPriorityTests.cs index 0c104d8..58a7ea5 100644 --- a/WitcherScriptMerger.Tests/LoadOrder/ModPriorityTests.cs +++ b/WitcherScriptMerger.Tests/LoadOrder/ModPriorityTests.cs @@ -1,4 +1,5 @@ -using System.Collections.Generic; +using System; +using System.Collections.Generic; using WitcherScriptMerger.LoadOrder; using Xunit; @@ -163,6 +164,43 @@ public void WithoutRankingEntry_RemovesOnlyTheReservedKey() Assert.Single(stripped); } + // Regression: a plain ToDictionary rebuilds with the default ordinal comparer and + // silently undoes the case-insensitive one WsmMcpTools.MergeConflicts deliberately + // installs, turning correctly-but-differently-cased path keys into no-ops. Only + // reachable once a ranking is supplied, so it would have hidden until the feature + // was actually used. + [Fact] + public void WithoutRankingEntry_PreservesACaseInsensitiveKeyComparer() + { + var overrides = new Dictionary(StringComparer.OrdinalIgnoreCase) + { + [ModPriority.OrderFileKey] = new[] { "modWinner" }, + [@"game\actor.ws"] = new[] { "modA", "modB" }, + }; + + var stripped = ModPriority.WithoutRankingEntry(overrides); + + Assert.True(stripped.ContainsKey(@"GAME\ACTOR.WS")); + } + + // ...and does not IMPOSE one where the caller never asked for it: the CLI hosts build + // their order dictionary with the default comparer, and lookups there must stay exactly + // as case-sensitive as they were before a ranking existed. + [Fact] + public void WithoutRankingEntry_DoesNotImposeACaseInsensitiveComparer() + { + var overrides = new Dictionary + { + [ModPriority.OrderFileKey] = new[] { "modWinner" }, + [@"game\actor.ws"] = new[] { "modA", "modB" }, + }; + + var stripped = ModPriority.WithoutRankingEntry(overrides); + + Assert.False(stripped.ContainsKey(@"GAME\ACTOR.WS")); + Assert.True(stripped.ContainsKey(@"game\actor.ws")); + } + [Fact] public void WithoutRankingEntry_NoReservedKey_ReturnsInputUnchanged() {