From 5e2e0c8a993097795660bc53294f8463b0d55142 Mon Sep 17 00:00:00 2001 From: Chris Knight Date: Fri, 21 Aug 2026 00:39:05 -0400 Subject: [PATCH] Exclude the merged mod from the conflict scan so re-merges stay idempotent The mod-directory filter honored only IgnoreModNames, so mod0000_MergedFiles - this tool's own output - matched BuildAsync's "mod*" glob and got scanned as an ordinary source mod. Every re-merge then fed the previous run's output back in alongside the mods it was built from, re-applying their edits on top of already-merged text: inserted blocks gained a fresh copy per run, and a losing most-distinct-from-vanilla tiebreak could revert an edit a previous run had kept. Found on a live 249-mod install after a routine re-merge: a single modBloodAndSteel insertion present 6x in actor.ws and a modCriSlowMoCR one 6x in damageManagerProcessor.ws (each appears exactly once in the mod's own file), 37 duplicated mod-added lines across 11 of 42 merged files, and one modTTMutagenSwap edit reverted outright. Nothing surfaced this as an error - the output stayed syntactically valid and merged "successfully" every time, which is why it went unnoticed across repeated merges. GetIgnoredModNames now also excludes MergedModName, via a new pure BuildIgnoredModNames(ignoreSetting, mergedNameSetting) that's unit-testable without touching AppState.Settings, and Paths.NormalizeMergedModName - a non-interactive counterpart to RetrieveMergedModName (which can prompt via ConfirmInvalidModName, and must not on a scan path) sharing its new MergedModNameMaxLength truncation so the excluded name always equals the directory a merge actually writes. Also drops the WinForms merge verb's bundle-tool gate: it used the combined ValidateDependencyPaths(), so `merge` refused to start at all without QuickBMS/wcc_lite even when every conflict was flat-file. Neither binary is committed to this repo, so a plain clone-and-run hit that refusal every time. Now matches Headless's text-merge-only gate; bundle conflicts still degrade gracefully per-conflict. 15 new tests (173 total). Verified A/B against origin/main on a scratch tree carrying a stale mod0000_MergedFiles whose marker strings appear in no mod and not in vanilla: pre-fix those markers leaked into the new output and the decision log named mod0000_MergedFiles as a merge participant; post-fix, zero leakage and no such decisions. Re-verified on the live install - a clean rebuild has 0 duplicated mod lines where the previous output had 37. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01FP8H6rBLCGPBFRSVsF3Kgw --- WitcherScriptMerger.Core/CLAUDE.md | 45 +++++ .../FileIndex/ModFileIndex.cs | 35 +++- WitcherScriptMerger.Core/Paths.cs | 34 +++- WitcherScriptMerger.Tests/CLAUDE.md | 12 ++ .../FileIndex/ModFileIndexTests.cs | 154 ++++++++++++++++++ WitcherScriptMerger/Program.cs | 17 +- 6 files changed, 289 insertions(+), 8 deletions(-) create mode 100644 WitcherScriptMerger.Tests/FileIndex/ModFileIndexTests.cs diff --git a/WitcherScriptMerger.Core/CLAUDE.md b/WitcherScriptMerger.Core/CLAUDE.md index be736a1..619cb2c 100644 --- a/WitcherScriptMerger.Core/CLAUDE.md +++ b/WitcherScriptMerger.Core/CLAUDE.md @@ -205,6 +205,42 @@ inconsistently — full path for the regex, trimmed segment for the allowlist related bug caught in the same review). Regression-tested via `FileMergerTests.IsVanillaDlcBundleFolder_FolderNameMerelyEndsInPattern_ReturnsFalse`. +## The merged mod is excluded from the conflict scan + +`ModFileIndex.BuildAsync` enumerates `Directory.GetDirectories(ModsDirectory, "mod*")` +and filters the result through `GetIgnoredModNames()`. That filter honors the +`IgnoreModNames` setting **and** always excludes the merged mod itself +(`MergedModName`, `mod0000_MergedFiles` by default). + +Excluding it is not cosmetic. The merged mod is this tool's own *output*, but its +directory name starts with `mod`, so it matches the same glob as any source mod. Left in +the scan it becomes a merge input alongside the very mods it was built from, and each +subsequent run re-applies those mods' edits on top of already-merged text — **a +re-merge becomes cumulative instead of idempotent**. Inserted blocks accumulate one fresh +copy per run, and a losing most-distinct-from-vanilla tiebreak can additionally revert an +edit a previous run had kept. Confirmed on a real 249-mod install before the fix: a single +`modBloodAndSteel` insertion present 6× in `actor.ws` and a `modCriSlowMoCR` one 6× in +`damageManagerProcessor.ws` (each appears exactly once in the mod's own file), 37 +duplicated mod-added lines across 11 of 42 merged files, and one `modTTMutagenSwap` edit +reverted outright. Nothing surfaced this as an error — the output stayed syntactically +valid and merged "successfully" every time, which is why it went unnoticed across +repeated merges. + +The name-matching lives in `Paths.NormalizeMergedModName(string)` — a non-interactive, +argument-taking counterpart to `Paths.RetrieveMergedModName()`. The scan path must not use +the latter: it can prompt via `ConfirmInvalidModName` and message through +`AppState.Notifier`, neither of which may fire just because mod directories are being +enumerated. Both apply the same `Paths.MergedModNameMaxLength` (64) truncation, which is +what decides the directory name a merge actually writes — the two must agree or a scan +would fail to recognize the very directory the merge creates. `NormalizeMergedModName` +additionally trims, deliberately: its result is compared against a `DirectoryInfo.Name`, +which never carries surrounding whitespace. + +`ModFileIndex.BuildIgnoredModNames(ignoreModNamesSetting, mergedModNameSetting)` is the +pure function behind `GetIgnoredModNames()`, split out so it's unit-testable without +touching `AppState.Settings` — see `WitcherScriptMerger.Tests/CLAUDE.md`'s +"`AppState.Settings`-safety constraints" and `FileIndex/ModFileIndexTests.cs`. + ## CLI & MCP orchestration (`Cli/`, `Mcp/`) `Cli/MergeOperations.cs` is the scan-then-merge sequence shared by both hosts' `merge` @@ -536,6 +572,15 @@ host's own `CLAUDE.md` for its own startup-level check; the MCP tools' own per-c (`RequireDependenciesAndModsDirectory`, above) always uses the text-merge-only check regardless of host. +Both hosts' `merge` CLI verbs now use the text-merge-only gate. The WinForms host's verb +previously used the combined `ValidateDependencyPaths()`, which made `merge` refuse to +start at all on an install whose conflicts were entirely flat-file (`.ws`/`.xml`) — the +common case, and the only category either headless path can resolve anyway. Since neither +QuickBMS nor wcc_lite is committed to this repo (see the root `CLAUDE.md`), a plain +clone-and-run hit that refusal every time, with an error message pointing at the GUI's +dependency setup for tooling the run didn't actually need. `ValidateDependencyPaths()` is +still there for callers that genuinely want the combined check. + ## Hash format (`MergeInventory.xml`) **Load-bearing.** `MergeInventory.xml` (including real, already-populated files on diff --git a/WitcherScriptMerger.Core/FileIndex/ModFileIndex.cs b/WitcherScriptMerger.Core/FileIndex/ModFileIndex.cs index 8185a7a..fb5f402 100644 --- a/WitcherScriptMerger.Core/FileIndex/ModFileIndex.cs +++ b/WitcherScriptMerger.Core/FileIndex/ModFileIndex.cs @@ -149,10 +149,39 @@ private List GetModFilesFromPaths( private IEnumerable GetIgnoredModNames() { - var ignoredNames = AppState.Settings.Get("IgnoreModNames"); - return ignoredNames.Split(',') + return BuildIgnoredModNames( + AppState.Settings.Get("IgnoreModNames"), + AppState.Settings.Get("MergedModName")); + } + + // Split out from GetIgnoredModNames (above) as a pure function over the two raw + // setting values so it's unit-testable without touching AppState.Settings - see + // WitcherScriptMerger.Tests/CLAUDE.md's "AppState.Settings-safety constraints". + // + // The merged mod is always excluded, on top of whatever the user configured in + // IgnoreModNames. It is this tool's own *output*, not a source mod, but its + // directory name starts with "mod" and so matches BuildAsync's "mod*" glob like any + // other. Left in the scan it becomes a merge input alongside the very mods it was + // built from, and each subsequent merge re-applies those mods' edits on top of an + // already-merged file - inserted blocks accumulate a fresh copy per run (observed + // live: a single modBloodAndSteel insertion present 6 times in actor.ws, and a + // modCriSlowMoCR one 6 times in damageManagerProcessor.ws, after repeated merges), + // and a losing tiebreak can additionally revert an edit a previous run had kept. + // Excluding it by name is what makes a re-merge idempotent instead of cumulative. + public static List BuildIgnoredModNames(string ignoreModNamesSetting, string mergedModNameSetting) + { + var ignoredNames = (ignoreModNamesSetting ?? string.Empty).Split(',') .Where(name => !string.IsNullOrWhiteSpace(name)) - .Select(name => name.Trim()); + .Select(name => name.Trim()) + .ToList(); + + var mergedModName = Paths.NormalizeMergedModName(mergedModNameSetting); + if (mergedModName != null && + !ignoredNames.Any(name => name.EqualsIgnoreCase(mergedModName))) + { + ignoredNames.Add(mergedModName); + } + return ignoredNames; } } } diff --git a/WitcherScriptMerger.Core/Paths.cs b/WitcherScriptMerger.Core/Paths.cs index 39e6750..1ee2318 100644 --- a/WitcherScriptMerger.Core/Paths.cs +++ b/WitcherScriptMerger.Core/Paths.cs @@ -162,6 +162,36 @@ public static string RetrieveMergedBundlePath() return null; } + // The merged mod's directory name is capped at this length; anything longer in the + // MergedModName setting is truncated to it, and that truncation is what decides the + // directory actually written to. Named (rather than left as a literal in + // RetrieveMergedModName) so NormalizeMergedModName below can apply the same cap + // without restating it - the two must agree, or a scan would fail to recognize the + // very directory a merge writes. + public const int MergedModNameMaxLength = 64; + + // Non-interactive counterpart to RetrieveMergedModName, for callers that only need + // to *recognize* the merged mod's directory name rather than resolve-and-validate a + // name to write to. Takes the raw setting value as an argument and touches neither + // AppState.Settings nor AppState.Notifier, so it's safe on a scan path (and directly + // unit-testable - see WitcherScriptMerger.Tests/CLAUDE.md's "AppState.Settings-safety + // constraints"): RetrieveMergedModName can prompt via ConfirmInvalidModName, which + // must never happen just because a scan is enumerating mod directories. + // + // Unlike RetrieveMergedModName this also trims, deliberately: the result is compared + // against a DirectoryInfo.Name, which never carries surrounding whitespace, so a + // setting value padded with spaces would otherwise silently fail to match and let the + // merged mod back into the scan - the exact failure this method exists to prevent. + public static string NormalizeMergedModName(string mergedModName) + { + if (string.IsNullOrWhiteSpace(mergedModName)) + return null; + mergedModName = mergedModName.Trim(); + return mergedModName.Length > MergedModNameMaxLength + ? mergedModName.Substring(0, MergedModNameMaxLength) + : mergedModName; + } + public static string RetrieveMergedModName() { var mergedModName = AppState.Settings.Get("MergedModName"); @@ -170,8 +200,8 @@ public static string RetrieveMergedModName() AppState.Notifier.ShowMessage("The MergedModName setting isn't configured in the .config file."); return null; } - if (mergedModName.Length > 64) - mergedModName = mergedModName.Substring(0, 64); + if (mergedModName.Length > MergedModNameMaxLength) + mergedModName = mergedModName.Substring(0, MergedModNameMaxLength); if (!mergedModName.IsAlphaNumeric() || !mergedModName.StartsWith("mod")) { if (!ConfirmInvalidModName(mergedModName)) diff --git a/WitcherScriptMerger.Tests/CLAUDE.md b/WitcherScriptMerger.Tests/CLAUDE.md index dd7fe05..bd1bd0f 100644 --- a/WitcherScriptMerger.Tests/CLAUDE.md +++ b/WitcherScriptMerger.Tests/CLAUDE.md @@ -45,6 +45,18 @@ does. in code review (a folder name merely *ending* in a recognized substring, e.g. `"ImmersiveDLC"`/`"Step1"`, must not match) — see Core's `CLAUDE.md`'s "Config-extensible vanilla-DLC-folder allowlist" section. +- `FileIndex/ModFileIndexTests.cs` — `ModFileIndex.BuildIgnoredModNames`, the pure + function behind the mod-directory filter `BuildAsync` applies to its `"mod*"` glob. + Regression coverage for the merged mod being scanned as an ordinary source mod, which + made every re-merge cumulative rather than idempotent (duplicated insertions, and + occasionally a reverted edit) — see Core's `CLAUDE.md`'s "The merged mod is excluded + from the conflict scan" section for the mechanism and the real-install evidence. Covers + the exclusion happening with no `IgnoreModNames` configured at all (the bug itself), a + non-default `MergedModName`, user entries surviving alongside it, case-insensitive + de-duplication when the merged mod is already listed by hand (the pre-fix workaround), + a blank/unconfigured `MergedModName` adding no phantom entry, and the + `Paths.MergedModNameMaxLength` truncation and whitespace-trimming that keep the excluded + name equal to the directory name a merge actually writes. - `LoadOrder/CustomLoadOrderTests.cs` — `CustomLoadOrder.ProcessLine`'s tolerance for `mods.settings` "VK=" (VortexKey) lines, via reflection — see Core's `CLAUDE.md`'s "Vortex-fork parity fixes" section. diff --git a/WitcherScriptMerger.Tests/FileIndex/ModFileIndexTests.cs b/WitcherScriptMerger.Tests/FileIndex/ModFileIndexTests.cs new file mode 100644 index 0000000..ee4ced0 --- /dev/null +++ b/WitcherScriptMerger.Tests/FileIndex/ModFileIndexTests.cs @@ -0,0 +1,154 @@ +using WitcherScriptMerger.FileIndex; +using Xunit; + +namespace WitcherScriptMerger.Tests.FileIndex +{ + // Regression coverage for ModFileIndex.BuildIgnoredModNames - the mod-directory filter + // BuildAsync applies to Directory.GetDirectories(ModsDirectory, "mod*"). + // + // Before this, that filter honored only the IgnoreModNames setting, so the merged mod + // (mod0000_MergedFiles by default) was scanned as an ordinary source mod: its name + // starts with "mod", so it matches the same glob. That made every re-merge cumulative + // rather than idempotent - the previous run's output became an input alongside the mods + // it was built from, so each run re-applied those mods' edits on top of already-merged + // text. Observed on a real 249-mod install: a single modBloodAndSteel insertion present + // 6 times in actor.ws and a modCriSlowMoCR one 6 times in damageManagerProcessor.ws + // (both appear exactly once in the mods' own files), 37 duplicated mod-added lines + // across 11 of 42 merged files, plus one modTTMutagenSwap edit reverted outright when + // the re-ingested output lost a most-distinct-from-vanilla tiebreak. + // + // These exercise the pure two-argument overload rather than GetIgnoredModNames, which + // reads AppState.Settings - see WitcherScriptMerger.Tests/CLAUDE.md's + // "AppState.Settings-safety constraints" for why tests must not touch that. Reaching + // Paths.NormalizeMergedModName/Paths.MergedModNameMaxLength through it is safe for the + // same reason those constraints exist: every static *field* initializer on Paths is + // settings-free by design (Path.Combine/literals only - see Paths.cs's own comment on + // why ScriptsDirectory et al. are properties, not cached fields), so touching a plain + // string helper there can't force AppState.Settings to construct and Environment.Exit + // the test host. + public class ModFileIndexTests + { + // The bug itself: nothing configured in IgnoreModNames must still not leave the + // merged mod in the scan. + [Fact] + public void BuildIgnoredModNames_NoIgnoreListConfigured_StillExcludesMergedMod() + { + var result = ModFileIndex.BuildIgnoredModNames("", "mod0000_MergedFiles"); + + Assert.Equal(new[] { "mod0000_MergedFiles" }, result); + } + + [Fact] + public void BuildIgnoredModNames_NullIgnoreList_StillExcludesMergedMod() + { + var result = ModFileIndex.BuildIgnoredModNames(null, "mod0000_MergedFiles"); + + Assert.Equal(new[] { "mod0000_MergedFiles" }, result); + } + + // A non-default MergedModName must be honored too - the exclusion follows the + // setting, not a hardcoded "mod0000_MergedFiles". + [Fact] + public void BuildIgnoredModNames_CustomMergedModName_ExcludesThatName() + { + var result = ModFileIndex.BuildIgnoredModNames("", "modAAA_MyMerges"); + + Assert.Equal(new[] { "modAAA_MyMerges" }, result); + } + + // The user's own IgnoreModNames entries keep working alongside the added exclusion. + [Fact] + public void BuildIgnoredModNames_WithIgnoreList_KeepsBothUserEntriesAndMergedMod() + { + var result = ModFileIndex.BuildIgnoredModNames("modFoo,modBar", "mod0000_MergedFiles"); + + Assert.Equal(new[] { "modFoo", "modBar", "mod0000_MergedFiles" }, result); + } + + [Fact] + public void BuildIgnoredModNames_IgnoreListEntriesAreTrimmed() + { + var result = ModFileIndex.BuildIgnoredModNames(" modFoo , modBar ", "mod0000_MergedFiles"); + + Assert.Equal(new[] { "modFoo", "modBar", "mod0000_MergedFiles" }, result); + } + + [Fact] + public void BuildIgnoredModNames_BlankIgnoreListEntriesAreDropped() + { + var result = ModFileIndex.BuildIgnoredModNames("modFoo,, ,modBar,", "mod0000_MergedFiles"); + + Assert.Equal(new[] { "modFoo", "modBar", "mod0000_MergedFiles" }, result); + } + + // Already listing the merged mod by hand (the pre-fix workaround) must not produce a + // duplicate entry, and must stay case-insensitive to match BuildAsync's own + // EqualsIgnoreCase comparison against DirectoryInfo.Name. + [Theory] + [InlineData("mod0000_MergedFiles")] + [InlineData("MOD0000_MERGEDFILES")] + [InlineData("Mod0000_mergedfiles")] + public void BuildIgnoredModNames_MergedModAlreadyInIgnoreList_NotDuplicated(string alreadyListed) + { + var result = ModFileIndex.BuildIgnoredModNames(alreadyListed, "mod0000_MergedFiles"); + + Assert.Single(result); + Assert.Equal(alreadyListed, result[0]); + } + + [Fact] + public void BuildIgnoredModNames_MergedModAlreadyListedAmongOthers_NotDuplicated() + { + var result = ModFileIndex.BuildIgnoredModNames("modFoo,mod0000_MergedFiles,modBar", "mod0000_MergedFiles"); + + Assert.Equal(new[] { "modFoo", "mod0000_MergedFiles", "modBar" }, result); + Assert.Single(result, name => name.EqualsIgnoreCase("mod0000_MergedFiles")); + } + + // An unconfigured/blank MergedModName must not add a phantom empty entry that would + // then match nothing (or, worse, everything). + [Theory] + [InlineData(null)] + [InlineData("")] + [InlineData(" ")] + public void BuildIgnoredModNames_NoMergedModNameConfigured_AddsNothing(string mergedModName) + { + var result = ModFileIndex.BuildIgnoredModNames("modFoo", mergedModName); + + Assert.Equal(new[] { "modFoo" }, result); + } + + [Theory] + [InlineData(null)] + [InlineData("")] + public void BuildIgnoredModNames_NothingConfiguredAtAll_ReturnsEmpty(string mergedModName) + { + Assert.Empty(ModFileIndex.BuildIgnoredModNames("", mergedModName)); + } + + // Surrounding whitespace in the setting must not stop the match: the comparison is + // against a DirectoryInfo.Name, which never carries any. + [Fact] + public void BuildIgnoredModNames_MergedModNamePadded_StillMatchesDirectoryName() + { + var result = ModFileIndex.BuildIgnoredModNames("", " mod0000_MergedFiles "); + + Assert.Equal(new[] { "mod0000_MergedFiles" }, result); + } + + // A MergedModName longer than the cap is truncated when the merge writes its output + // directory (Paths.RetrieveMergedModName), so the scan has to exclude the truncated + // name - excluding the untruncated one would miss the directory that actually exists. + [Fact] + public void BuildIgnoredModNames_OverlongMergedModName_ExcludesTheTruncatedName() + { + var overlong = "mod" + new string('A', WitcherScriptMerger.Paths.MergedModNameMaxLength); + var expected = overlong.Substring(0, WitcherScriptMerger.Paths.MergedModNameMaxLength); + + var result = ModFileIndex.BuildIgnoredModNames("", overlong); + + Assert.Equal(new[] { expected }, result); + Assert.Equal(WitcherScriptMerger.Paths.MergedModNameMaxLength, result[0].Length); + } + } +} diff --git a/WitcherScriptMerger/Program.cs b/WitcherScriptMerger/Program.cs index b40ceae..a76a2a4 100644 --- a/WitcherScriptMerger/Program.cs +++ b/WitcherScriptMerger/Program.cs @@ -180,11 +180,22 @@ static int RunCli(string[] args) return 1; } - if (!Paths.ValidateDependencyPaths()) + // Only the text-merge engine (DiffPlexMergeEngine, always available - it's + // in-process) gates starting a merge run, matching WitcherScriptMerger.Headless's + // own merge verb. This deliberately does NOT also require QuickBMS/wcc_lite via + // Paths.ValidateDependencyPaths(): those are needed only for bundle-content + // conflicts, and requiring them up front made `merge` refuse to run at all on an + // install that has nothing but flat-file (.ws/.xml) conflicts - the common case, + // and the only case either headless path can resolve anyway. Neither binary is + // committed to this repo (see the root CLAUDE.md), so a plain clone-and-run hit + // this every time. Bundle-category conflicts still fail gracefully, per-conflict, + // when actually attempted without the tooling configured - see + // ModFileIndex.BuildAsync and FileMerger.GetUnpackedFiles (Core). + if (!Paths.ValidateTextMergeDependencies()) { Notifier.ShowError( - "A required dependency (QuickBMS or wcc_lite) is missing. Configure its path " + - "in App.config, or run without arguments once to use the GUI's dependency setup."); + "The configured text-merge engine is missing or misconfigured. This shouldn't " + + "happen with the built-in DiffPlex engine - check for a corrupted install."); return 1; }