From 641820da1472d3439391c997d6c57f763802c325 Mon Sep 17 00:00:00 2001 From: Chris Knight Date: Tue, 25 Aug 2026 23:09:25 -0400 Subject: [PATCH 1/2] Stop re-reporting the same conflicts as needing manual resolution A live 350-mod run reported three conflicts as "needs manual resolution" on every single pass, with no way to make it stop. The merge engine was right to decline all three - each has a source mod shipping a whole-file copy of a vanilla script taken from an older game build, so merging that side would delete vanilla code the game and the other mod still call. But nothing said which mod to fix, and two separate gaps guaranteed the same files came back. Four changes, all found together: - Tools/StaleBuildDetector.cs runs the vanilla-declaration comparison as a pre-flight, off the files, before any merge, so a warning names the stale mod and the remedy (update or disable it) instead of pointing at a 2500-line file to hand-merge. It's a diagnostic, never a gate - its message reports drift and its usual consequence rather than predicting this conflict will fail, because it can't know that: a real install had one mod missing 1 of 224 declarations whose conflict auto-solved every time, and another missing 13 that didn't. - MergeInventory.HasResolvedConflict now requires the merged output to exist. Every hash it checked belonged to a source mod - all present and unchanged when it's the output that was deleted - so a record whose merged file was gone answered "resolved" forever and nothing re-merged it, leaving the game to load exactly one of two conflicting mods with nothing saying so. The GUI already refused to trust such a record; Inventory/MergeInventoryHygiene.cs lifts that rule and its two siblings out so the CLI, MCP and Vortex extension see them too. They report rather than act: the GUI's prompts pass no defaultResult, so HeadlessMergeNotifier would answer its generic YesNo => No and prune nothing. - ModFileIndex now skips mod folders mods.settings marks Enabled=0. A scan is a filesystem glob, so a deployed-but-disabled mod counted as a full conflict participant and could keep a conflict permanently unmergeable - which is exactly what one of the three was. Only an explicit Enabled=0 excludes a mod; an absent or unreadable mods.settings disables nobody. MergeDisabledMods opts out, named for the opt-out so an older App.config yields the new behavior. - HeadlessMergeNotifier.RouteAllOutputToStandardError, set by both hosts' mcp verb. Only Error/Warning/Exclamation went to stderr, so any default-icon message a scan emits landed on stdout - mid-JSON-RPC-stream in MCP mode. Caught by a real scan_conflicts round-trip coming back unparseable, with the new disabled-mod notice spliced between two protocol frames. Pre-existing: BuildAsync's "Can't find any mods" had the same shape. Verified against the live install: 44 conflicts with 3 unmergeable and one inventory record with no output file, before; 43 conflicts and 43/43 resolved with zero skips, after. Re-running with WSM_MergeDisabledMods=true brings the phantom back and prints the stale-build warning naming the mod and 13 of the 224 declarations it lacks. 239 tests pass; format check clean. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01N6y39x9F3SKcS4hhpw85Yi --- .../FileIndex/ModFileIndex.cs | 98 +++++++++ .../HeadlessMergeNotifier.cs | 23 +- .../Inventory/MergeInventory.cs | 18 ++ .../Inventory/MergeInventoryHygiene.cs | 161 ++++++++++++++ WitcherScriptMerger.Core/Mcp/WsmMcpTools.cs | 47 +++- .../Tools/StaleBuildDetector.cs | 176 +++++++++++++++ WitcherScriptMerger.Headless/App.config | 10 + WitcherScriptMerger.Headless/Program.cs | 16 ++ .../Inventory/MergeInventoryHygieneTests.cs | 181 +++++++++++++++ .../Tools/StaleBuildDetectorTests.cs | 207 ++++++++++++++++++ WitcherScriptMerger/App.config | 10 + WitcherScriptMerger/Program.cs | 16 ++ 12 files changed, 960 insertions(+), 3 deletions(-) create mode 100644 WitcherScriptMerger.Core/Inventory/MergeInventoryHygiene.cs create mode 100644 WitcherScriptMerger.Core/Tools/StaleBuildDetector.cs create mode 100644 WitcherScriptMerger.Tests/Inventory/MergeInventoryHygieneTests.cs create mode 100644 WitcherScriptMerger.Tests/Tools/StaleBuildDetectorTests.cs diff --git a/WitcherScriptMerger.Core/FileIndex/ModFileIndex.cs b/WitcherScriptMerger.Core/FileIndex/ModFileIndex.cs index fb5f402..ae06921 100644 --- a/WitcherScriptMerger.Core/FileIndex/ModFileIndex.cs +++ b/WitcherScriptMerger.Core/FileIndex/ModFileIndex.cs @@ -4,6 +4,7 @@ using System.IO; using System.Linq; using WitcherScriptMerger.Inventory; +using WitcherScriptMerger.LoadOrder; using WitcherScriptMerger.Tools; namespace WitcherScriptMerger.FileIndex @@ -38,6 +39,9 @@ public void BuildAsync( var modDirPaths = Directory.GetDirectories(Paths.ModsDirectory, "mod*", SearchOption.TopDirectoryOnly) .Where(path => !ignoredModNames.Any(name => name.EqualsIgnoreCase(new DirectoryInfo(path).Name))) .ToList(); + + modDirPaths = ExcludeDisabledMods(modDirPaths); + ModCount = modDirPaths.Count; if (ModCount == 0) { @@ -154,6 +158,100 @@ private IEnumerable GetIgnoredModNames() AppState.Settings.Get("MergedModName")); } + // Drops mod folders the game will never load because mods.settings marks them + // Enabled=0. + // + // A scan is a filesystem glob over Paths.ModsDirectory, so a mod that is deployed + // but disabled looked exactly like an active one, and its files were counted as + // full conflict participants. That is not a cosmetic over-count: a disabled mod + // can make a conflict genuinely unmergeable and keep it that way forever. Observed + // live - a disabled modFearlessRoach ships a pre-next-gen whole-file copy of + // game\vehicles\horse\states\exploration.ws (missing CheckVector / DoHorseKick / + // OnHorseKick), which trips FunctionLevelMergeEngine's vanilla-declaration + // invariant, so every run reported "needs manual resolution" for a file the game + // was never going to load that mod's version of anyway. + // + // Only an EXPLICIT Enabled=0 excludes a mod. CustomLoadOrder.IsModDisabledByName + // returns false for a mod that isn't in mods.settings at all, which is the correct + // reading: the game appends unknown mod folders on next launch, enabled. A missing + // or unreadable mods.settings likewise disables no one (CustomLoadOrder.Refresh + // leaves Mods empty), so this can never make a scan miss conflicts on a fresh + // install, or on a Linux host with no Documents\The Witcher 3 at all. + // + // Set MergeDisabledMods=true to opt out and go back to scanning every deployed mod + // - the useful case being pre-merging a mod that's staged but not switched on yet. + // The setting is named for the opt-OUT so that its absence from an older + // App.config (AppSettings.Get yields false for a key that isn't there) means + // the new, wanted behavior rather than silently keeping the old one. + private List ExcludeDisabledMods(List modDirPaths) + { + if (AppState.Settings.Get("MergeDisabledMods")) + return modDirPaths; + + // Built here rather than taken from AppState.LoadOrder because the MCP tools + // never populate that field (only the CLI verb and the GUI do), and a scan + // must not depend on which entry point it was reached from. + CustomLoadOrder loadOrder; + try + { + loadOrder = AppState.LoadOrder ?? new CustomLoadOrder(); + } + catch (Exception) + { + // mods.settings unreadable - exclude nothing, exactly as if every mod were enabled. + return modDirPaths; + } + + var kept = ExcludeDisabledModPaths(modDirPaths, loadOrder.IsModDisabledByName, out var skipped); + DisabledModsSkipped = skipped; + + // Reported, never silent: a conflict vanishing from the scan because a mod is + // switched off should be visible in the run's output, not something the user + // has to infer from a changed conflict count. + if (DisabledModsSkipped.Count > 0) + { + AppState.Notifier.ShowMessage( + $"Skipped {DisabledModsSkipped.Count} disabled mod folder(s) - the game won't load them, so they can't " + + $"conflict: {string.Join(", ", DisabledModsSkipped)}. Set MergeDisabledMods=true to include them anyway.", + "Disabled Mods Skipped"); + } + + return kept; + } + + // Split out from ExcludeDisabledMods (above) as a pure function over a + // path list and a name predicate, so it's unit-testable without touching + // AppState.Settings or reading a real mods.settings - the same shape, and the same + // reason, as BuildIgnoredModNames just above. See + // WitcherScriptMerger.Tests/CLAUDE.md's "AppState.Settings-safety constraints". + // + // The predicate is asked about the mod's FOLDER NAME, not its full path: that's the + // name mods.settings keys on and the name the game itself loads by. + public static List ExcludeDisabledModPaths( + IEnumerable modDirPaths, Func isModDisabled, out List skippedModNames) + { + skippedModNames = new List(); + var kept = new List(); + if (modDirPaths == null) + return kept; + + foreach (var path in modDirPaths) + { + var name = Path.GetFileName(path); + if (isModDisabled != null && isModDisabled(name)) + skippedModNames.Add(name); + else + kept.Add(path); + } + + skippedModNames.Sort(StringComparer.OrdinalIgnoreCase); + return kept; + } + + // Mod folder names left out of the most recent scan by ExcludeDisabledMods. + // Never null; empty when nothing was skipped. + public IReadOnlyList DisabledModsSkipped { get; private set; } = new List(); + // 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". diff --git a/WitcherScriptMerger.Core/HeadlessMergeNotifier.cs b/WitcherScriptMerger.Core/HeadlessMergeNotifier.cs index f43deac..db87999 100644 --- a/WitcherScriptMerger.Core/HeadlessMergeNotifier.cs +++ b/WitcherScriptMerger.Core/HeadlessMergeNotifier.cs @@ -6,7 +6,7 @@ namespace WitcherScriptMerger // every decision has a fixed, non-destructive default (don't overwrite, // don't use a conflicting merge name, don't retry) so a batch run can // never hang waiting for a prompt nobody is watching. - class HeadlessMergeNotifier : IMergeNotifier + public class HeadlessMergeNotifier : IMergeNotifier { public NotifyResult ShowMessage(string text, string title = "", @@ -47,12 +47,31 @@ public NotifyResult ShowError(string text, string title = "Error") return NotifyResult.OK; } + // Set by both hosts' `mcp` verb before the server starts. In MCP mode stdout + // carries the JSON-RPC frame stream and nothing else - a notifier line printed + // there lands mid-stream and corrupts the transport, which the client sees as + // unparseable JSON rather than as a message from WSM. + // + // Found the hard way: a new advisory message in ModFileIndex (the disabled-mod + // skip notice) used the default DialogIcon.None, so Write sent it to stdout, and + // it appeared spliced between two protocol frames in a real `scan_conflicts` + // round-trip. The same hazard already existed for every other default-icon + // ShowMessage a scan can reach - notably BuildAsync's "Can't find any mods in the + // Mods directory." - so this is fixed here, once, rather than by picking a + // stderr-routed icon at each call site and calling the semantics close enough. + // + // CLI mode is unaffected: the flag stays false there, so ordinary progress output + // still goes to stdout and only Error/Warning/Exclamation go to stderr, exactly + // as before. + public static bool RouteAllOutputToStandardError; + static void Write(string text, string title, DialogIcon icon) { var prefix = string.IsNullOrEmpty(title) ? "WSM" : title; var line = $"[{prefix}] {text}"; - if (icon == DialogIcon.Error || icon == DialogIcon.Warning || icon == DialogIcon.Exclamation) + if (RouteAllOutputToStandardError + || icon == DialogIcon.Error || icon == DialogIcon.Warning || icon == DialogIcon.Exclamation) Console.Error.WriteLine(line); else Console.WriteLine(line); diff --git a/WitcherScriptMerger.Core/Inventory/MergeInventory.cs b/WitcherScriptMerger.Core/Inventory/MergeInventory.cs index 6428254..07aa874 100644 --- a/WitcherScriptMerger.Core/Inventory/MergeInventory.cs +++ b/WitcherScriptMerger.Core/Inventory/MergeInventory.cs @@ -117,6 +117,24 @@ public bool HasResolvedConflict(ModFile conflict) if (merge == null) return false; + // A record whose merged output isn't on disk has not resolved anything, whatever + // else it claims. Without this the record is self-certifying: every hash below + // still matches (the SOURCE mods are all present and unchanged - it's the OUTPUT + // that's gone), so this returned true forever, the conflict was reported + // alreadyResolved on every scan, and nothing ever re-merged it. Observed on a + // real install for game\vehicles\horse\states\exploration.ws: record present, + // merged file absent, and the game therefore loading exactly one of the two + // conflicting mods with no indication anything was wrong. + // + // The WinForms GUI already refused to trust such a record - MainForm's + // RefreshMergeTree() prunes it via ConfirmPruneMissingMergeFile - but that check + // lived only in the GUI, so the CLI, the MCP tools and the Vortex extension + // (which drives the headless host) never saw it. See MergeInventoryHygiene, + // which is where that rule and its two siblings now live for every caller; + // HasMergedFile also explains why bundle-content records are exempt. + if (!MergeInventoryHygiene.HasMergedFile(merge)) + return false; + if (conflict.Mods.Any(mod => !mod.Name.EqualsIgnoreCase(merge.MergedModName) && !merge.ContainsMod(mod.Name))) return false; diff --git a/WitcherScriptMerger.Core/Inventory/MergeInventoryHygiene.cs b/WitcherScriptMerger.Core/Inventory/MergeInventoryHygiene.cs new file mode 100644 index 0000000..27953c9 --- /dev/null +++ b/WitcherScriptMerger.Core/Inventory/MergeInventoryHygiene.cs @@ -0,0 +1,161 @@ +using System; +using System.Collections.Generic; +using System.IO; +using System.Linq; +using WitcherScriptMerger.FileIndex; +using WitcherScriptMerger.LoadOrder; + +namespace WitcherScriptMerger.Inventory +{ + public enum StaleMergeReason + { + // The record's own output file isn't on disk. Until this was checked, + // MergeInventory.HasResolvedConflict answered "resolved" for such a record + // forever, so the conflict was never re-merged and the game silently loaded + // whichever single mod won load order. + MergedFileMissing, + + // A mod the record lists no longer has the file (uninstalled, or the file was + // dropped by a mod update). + SourceModFileMissing, + + // A mod the record lists is explicitly disabled in mods.settings, so the game + // never loads it - its contribution to the merged output is dead weight. + SourceModDisabled, + } + + public sealed class StaleMergeRecord + { + public Merge Merge { get; } + public StaleMergeReason Reason { get; } + + // The mod that triggered the finding; null for MergedFileMissing, which is about + // the record's output rather than any one source. + public string ModName { get; } + + public StaleMergeRecord(Merge merge, StaleMergeReason reason, string modName = null) + { + Merge = merge; + Reason = reason; + ModName = modName; + } + + public string Describe() + { + switch (Reason) + { + case StaleMergeReason.MergedFileMissing: + return $"{Merge.RelativePath}: recorded as merged, but the merged file is missing " + + $"({SafeMergedPath()}) - the conflict is unresolved despite the inventory record."; + case StaleMergeReason.SourceModFileMissing: + return $"{Merge.RelativePath}: recorded source mod '{ModName}' no longer has this file."; + case StaleMergeReason.SourceModDisabled: + return $"{Merge.RelativePath}: recorded source mod '{ModName}' is disabled in mods.settings, " + + "so its changes are merged in but the game would never have loaded them."; + default: + return $"{Merge.RelativePath}: stale ({Reason})."; + } + } + + string SafeMergedPath() + { + try { return Merge.GetMergedFile(); } + catch (Exception) { return "path unavailable"; } + } + + public override string ToString() => Describe(); + } + + // The inventory-hygiene rules the WinForms GUI has always applied in + // MainForm.RefreshMergeTree() - "merged file missing", "source mod file missing", + // "source mod disabled" - lifted out as pure, promptless predicates so the headless + // CLI, the MCP tools and the Vortex extension can see the same staleness the GUI + // does. + // + // Before this existed those three rules were GUI-only, which had a concrete + // consequence on a real install: a record for + // game\vehicles\horse\states\exploration.ws outlived its output file, and because + // HasResolvedConflict never checked that the output existed, every headless scan kept + // answering alreadyResolved:true for a conflict that had no merged file at all. + // + // Note these are findings, not actions. Deciding what to do about a stale record is + // the caller's - the GUI asks the user (ConfirmPruneMissingMergeFile and friends); + // headless callers report it. That split is deliberate: the GUI's prompts pass no + // defaultResult, so HeadlessMergeNotifier would answer its generic YesNo => No to + // every one of them and prune nothing, silently. + public static class MergeInventoryHygiene + { + // Whether a record's merged output is on disk. + // + // Bundle-content records are always reported as present. Their GetMergedFile() + // resolves under Paths.MergedBundleContent, which is relative to the current + // working directory and is scratch space between runs - absence there says + // nothing about whether the merge is live, since the real artifact is the packed + // bundle. Only flat files (.ws/.xml) have a stable, absolute output path under + // the mods directory that "missing" is meaningful for, and flat files are the + // only category either headless host supports at all. + public static bool HasMergedFile(Merge merge) + { + if (merge == null) + return true; + if (merge.Category != Categories.Script && merge.Category != Categories.Xml) + return true; + + try + { + return File.Exists(merge.GetMergedFile()); + } + catch (Exception ex) when (ex is IOException || ex is UnauthorizedAccessException || ex is ArgumentException) + { + // Can't prove it's missing - don't claim it is. + return true; + } + } + + // Every stale record in the inventory. A single record can yield more than one + // finding (e.g. a missing output whose source mod is also disabled); callers that + // only want one line per record can group by Merge. + // + // loadOrder may be null - the SourceModDisabled rule is simply not applied then, + // which is the right behavior when mods.settings can't be read (a fresh install, + // or a Linux host with no Documents\The Witcher 3 at all). CustomLoadOrder + // already treats a mod absent from mods.settings as enabled, so a mod the game + // would auto-add on next launch is never reported as disabled. + public static IReadOnlyList FindStale(MergeInventory inventory, CustomLoadOrder loadOrder = null) + { + var stale = new List(); + if (inventory?.Merges == null) + return stale; + + foreach (var merge in inventory.Merges) + { + if (merge == null) + continue; + + if (!HasMergedFile(merge)) + stale.Add(new StaleMergeRecord(merge, StaleMergeReason.MergedFileMissing)); + + foreach (var mod in merge.Mods ?? Enumerable.Empty()) + { + if (mod?.Name == null) + continue; + + try + { + if (!File.Exists(merge.GetModFile(mod.Name))) + stale.Add(new StaleMergeRecord(merge, StaleMergeReason.SourceModFileMissing, mod.Name)); + } + catch (Exception ex) when (ex is IOException || ex is UnauthorizedAccessException || ex is ArgumentException || ex is NotImplementedException) + { + // Same reasoning as HasMergedFile: an unresolvable path isn't proof of absence. + } + + if (loadOrder != null && loadOrder.IsModDisabledByName(mod.Name)) + stale.Add(new StaleMergeRecord(merge, StaleMergeReason.SourceModDisabled, mod.Name)); + } + } + + return stale; + } + } +} diff --git a/WitcherScriptMerger.Core/Mcp/WsmMcpTools.cs b/WitcherScriptMerger.Core/Mcp/WsmMcpTools.cs index 450624d..4fec3c9 100644 --- a/WitcherScriptMerger.Core/Mcp/WsmMcpTools.cs +++ b/WitcherScriptMerger.Core/Mcp/WsmMcpTools.cs @@ -7,6 +7,7 @@ using WitcherScriptMerger.Cli; using WitcherScriptMerger.Inventory; using WitcherScriptMerger.LoadOrder; +using WitcherScriptMerger.Tools; namespace WitcherScriptMerger.Mcp { @@ -43,6 +44,14 @@ public static object ScanConflicts() mods = c.Mods.Select(h => new { name = h.Name, hash = h.Hash, isOutdated = h.IsOutdated }).ToArray(), defaultOrder = c.Mods.Select(h => h.Name).OrderBy(n => n, new LoadOrderComparer()).ToArray(), alreadyResolved = AppState.Inventory.HasResolvedConflict(c), + + // Per-conflict rather than a separate top-level section, so this array + // stays the whole payload and existing callers keep parsing it unchanged. + // Non-empty means at least one source mod's copy of this file predates the + // installed game build, which is the usual reason a conflict can't be + // auto-merged - and points at the mod to update rather than at a file to + // hand-merge. See StaleBuildDetector. + staleBuildWarnings = StaleBuildDetector.Analyze(c).Select(f => f.Describe()).ToArray(), }).ToArray(); } } @@ -152,7 +161,25 @@ public static object MergeConflicts( if (!dryRun) AppState.Inventory.Save(); - return new { merged = summary.Merged, skipped = summary.Skipped, unmatched, dryRun, overwrite, functionLevelDecisions = summary.FunctionLevelDecisions }; + // Computed over the conflicts actually in scope, and returned whether or not + // anything was skipped: when a skip IS present these usually name its real + // cause, and when nothing was skipped they're a standing heads-up that a mod + // is drifting behind the game build. Not gated on summary.Skipped for that + // second reason - a stale mod can merge fine right up until another mod + // starts touching the same declarations. + var staleBuild = StaleBuildDetector.Analyze(conflicts).Select(f => f.Describe()).ToArray(); + + return new + { + merged = summary.Merged, + skipped = summary.Skipped, + unmatched, + dryRun, + overwrite, + functionLevelDecisions = summary.FunctionLevelDecisions, + staleBuildWarnings = staleBuild, + disabledModsSkipped = modIndex.DisabledModsSkipped, + }; } } @@ -200,11 +227,29 @@ public static object ListMerges() { var inventory = MergeInventory.Load(Paths.Inventory); + // One CustomLoadOrder for the whole listing rather than one per record. Null + // if mods.settings can't be read at all, which MergeInventoryHygiene handles by + // simply not applying its disabled-mod rule. + CustomLoadOrder loadOrder; + try { loadOrder = new CustomLoadOrder(); } + catch (Exception) { loadOrder = null; } + + var staleByPath = MergeInventoryHygiene.FindStale(inventory, loadOrder) + .GroupBy(s => s.Merge.RelativePath, StringComparer.OrdinalIgnoreCase) + .ToDictionary(g => g.Key, g => g.Select(s => s.Describe()).ToArray(), StringComparer.OrdinalIgnoreCase); + return inventory.Merges.Select(m => new { relativePath = m.RelativePath, mergedModName = m.MergedModName, mods = m.Mods.Select(h => new { name = h.Name, hash = h.Hash }).ToArray(), + + // A record is not evidence that a merge is live - its output file can be + // deleted, its source mods uninstalled or switched off, all without the + // record changing. Surfacing that here means a headless caller can see the + // same staleness the GUI prunes on every refresh. + mergedFileExists = MergeInventoryHygiene.HasMergedFile(m), + staleWarnings = staleByPath.TryGetValue(m.RelativePath ?? string.Empty, out var w) ? w : Array.Empty(), }).ToArray(); } diff --git a/WitcherScriptMerger.Core/Tools/StaleBuildDetector.cs b/WitcherScriptMerger.Core/Tools/StaleBuildDetector.cs new file mode 100644 index 0000000..4ddaf69 --- /dev/null +++ b/WitcherScriptMerger.Core/Tools/StaleBuildDetector.cs @@ -0,0 +1,176 @@ +using System; +using System.Collections.Generic; +using System.IO; +using System.Linq; +using WitcherScriptMerger.FileIndex; + +namespace WitcherScriptMerger.Tools +{ + // A single mod's script file that is missing declarations the CURRENT vanilla file + // has - the "whole-file copy taken from an older game build" signature. + public sealed class StaleBuildFinding + { + public string RelativePath { get; } + public string ModName { get; } + + // ScriptUnitExtractor scoped names (e.g. "CR4Game::OnHDRChangedEvent"), the same + // identity FunctionLevelMergeEngine's invariant reports, so a pre-flight warning + // and the post-hoc violation name the same thing. + public IReadOnlyList MissingDeclarations { get; } + + public int VanillaDeclarationCount { get; } + + public StaleBuildFinding(string relativePath, string modName, IReadOnlyList missing, int vanillaDeclarationCount) + { + RelativePath = relativePath; + ModName = modName; + MissingDeclarations = missing; + VanillaDeclarationCount = vanillaDeclarationCount; + } + + public string Describe() + { + var names = string.Join(", ", MissingDeclarations.Take(StaleBuildDetector.MaxNamesInDescription)); + if (MissingDeclarations.Count > StaleBuildDetector.MaxNamesInDescription) + names += $", +{MissingDeclarations.Count - StaleBuildDetector.MaxNamesInDescription} more"; + + // Deliberately does NOT claim the conflict can't be merged. This check runs + // before any merge and can't know that: a small drift often merges fine (a + // real install had a mod missing 1 of 224 declarations whose conflict + // auto-solved every time), while a large one reliably trips the + // vanilla-declaration invariant. Stating the fact and its usual consequence + // keeps the warning honest at both ends of that range. + return + $"{ModName} ships a copy of {RelativePath} built against an older game version - " + + $"it is missing {MissingDeclarations.Count} of the {VanillaDeclarationCount} declaration(s) " + + $"the installed vanilla file has ({names}). Merging that mod's side can silently delete vanilla " + + "code the game and other mods still call, which is the usual reason a conflict here needs manual " + + "resolution. Update the mod to a build matching your game version, or disable it."; + } + + public override string ToString() => Describe(); + } + + // Pre-flight counterpart to FunctionLevelMergeEngine's vanilla-declaration invariant. + // + // That invariant is the safety net: it fires AFTER a merge has been attempted and + // produced output that would have dropped vanilla declarations, and its message + // correctly guesses the cause ("...usually means that mod ships a whole-file copy + // taken from an older game build"). But by then the user is looking at a "Skipped - + // needs manual resolution" line, and the actionable fact - WHICH mod is out of date, + // and that the remedy is to update/disable that mod rather than hand-merge a 2500-line + // file - is buried inside a sentence about a DiffPlex bug. + // + // This runs the same comparison up front, straight off the files, with no merge + // involved: every declaration the installed vanilla file has, that a given mod's copy + // does not. It is deliberately a diagnostic, never a gate - a mod MAY legitimately + // delete a vanilla function, and that case is rare enough (and interesting enough) + // that reporting it and letting the merge proceed is the right trade. Nothing here + // changes what does or doesn't merge. + // + // Verified against a real 350-mod, game-build-4.04 install: 136 mod-copy comparisons + // across 44 conflicts produced exactly 3 findings, and those 3 were precisely the 3 + // conflicts the engine's invariant went on to decline (modFearlessRoach/exploration.ws, + // modFastTravelFromAnywhere/mapMenu.ws, modAlwaysFullExp/r4Game.ws) - no false + // positives, no misses. + public static class StaleBuildDetector + { + public const int MaxNamesInDescription = 6; + + // Scoped names of every unit in `text`, or null if it doesn't scan cleanly. + // Null (rather than an empty set) matters: an unscannable file must produce NO + // finding, whereas an empty set would make every vanilla declaration look missing. + static HashSet ScopedNames(string text) + { + try + { + return new HashSet( + ScriptUnitExtractor.Extract(text).Units.Select(u => u.ScopedName), + StringComparer.Ordinal); + } + catch (ScriptUnitExtractor.ExtractionException) + { + return null; + } + } + + // Declarations present in vanillaText but absent from modText. Empty when either + // side can't be scanned - see ScopedNames. + public static IReadOnlyList FindMissingVanillaDeclarations(string vanillaText, string modText) + { + var vanilla = ScopedNames(vanillaText); + var mod = ScopedNames(modText); + if (vanilla == null || mod == null) + return Array.Empty(); + + return vanilla.Where(name => !mod.Contains(name)).OrderBy(name => name, StringComparer.Ordinal).ToList(); + } + + // One conflict's findings, one per source mod whose copy is missing vanilla + // declarations. Only .ws conflicts are examined - ScriptUnitExtractor is + // WitcherScript-specific and has no notion of XML or bundle content, exactly as + // DiffPlexMergeEngine's own function-level rescue is gated. + public static IReadOnlyList Analyze(ModFile conflict) + { + if (conflict == null || conflict.Category != Categories.Script) + return Array.Empty(); + + string vanillaText; + int vanillaCount; + try + { + var vanillaPath = conflict.GetVanillaFile(); + if (!File.Exists(vanillaPath)) + return Array.Empty(); + + vanillaText = FileEncoding.ReadAnyEncoding(vanillaPath); + var vanillaNames = ScopedNames(vanillaText); + if (vanillaNames == null) + return Array.Empty(); + vanillaCount = vanillaNames.Count; + } + catch (Exception ex) when (ex is IOException || ex is UnauthorizedAccessException) + { + return Array.Empty(); + } + + // The merged mod folder routinely shows up among a conflict's sources once a + // file has been merged once. It is this tool's own output, not a source mod + // shipped by anyone, so an "older game build" verdict on it would be both + // meaningless and alarming. + var mergedModName = Paths.RetrieveMergedModName(); + + var findings = new List(); + foreach (var mod in conflict.Mods) + { + if (mod?.Name == null || mod.Name.EqualsIgnoreCase(mergedModName)) + continue; + + try + { + var modPath = conflict.GetModFile(mod.Name); + if (!File.Exists(modPath)) + continue; + + var missing = FindMissingVanillaDeclarations(vanillaText, FileEncoding.ReadAnyEncoding(modPath)); + if (missing.Count > 0) + findings.Add(new StaleBuildFinding(conflict.RelativePath, mod.Name, missing, vanillaCount)); + } + catch (Exception ex) when (ex is IOException || ex is UnauthorizedAccessException) + { + // A mod file that can't be read is not evidence of a stale build. + } + } + + return findings; + } + + public static IReadOnlyList Analyze(IEnumerable conflicts) + { + if (conflicts == null) + return Array.Empty(); + + return conflicts.SelectMany(Analyze).ToList(); + } + } +} diff --git a/WitcherScriptMerger.Headless/App.config b/WitcherScriptMerger.Headless/App.config index 5e710ef..c8db724 100644 --- a/WitcherScriptMerger.Headless/App.config +++ b/WitcherScriptMerger.Headless/App.config @@ -16,6 +16,15 @@ CheckBundleContents Whether to check for mod conflicts in bundle file conten than crashing if this is turned on regardless. IgnoreModNames Which mod folders to ignore (separated by commas) +MergeDisabledMods Whether to scan mod folders that mods.settings marks Enabled=0. + Defaults to false: the game never loads a disabled mod, so it can't + really conflict with anything, and counting it anyway can keep a + conflict permanently unmergeable (a disabled mod shipping a whole-file + copy from an older game build trips the vanilla-declaration invariant + on every run). Set to true to pre-merge mods that are staged but not + switched on yet. A mod absent from mods.settings counts as enabled - + only an explicit Enabled=0 excludes it. + AdditionalVanillaDlcFolderNames Extra vanilla DLC/expansion folder names (under GameDirectory\DLC) to treat as a 3-way merge's vanilla baseline, in addition to the @@ -50,6 +59,7 @@ since KDiff3MergeEngine needs Win32 P/Invoke that isn't available outside the Wi + diff --git a/WitcherScriptMerger.Headless/Program.cs b/WitcherScriptMerger.Headless/Program.cs index 98172ce..8fa7721 100644 --- a/WitcherScriptMerger.Headless/Program.cs +++ b/WitcherScriptMerger.Headless/Program.cs @@ -10,6 +10,7 @@ using WitcherScriptMerger.Inventory; using WitcherScriptMerger.LoadOrder; using WitcherScriptMerger.Mcp; +using WitcherScriptMerger.Tools; namespace WitcherScriptMerger.Headless { @@ -163,6 +164,15 @@ static int RunMerge(string[] args) foreach (var decision in summary.FunctionLevelDecisions) Console.WriteLine($" function-level: {decision}"); + // Printed after the skip list on purpose: for a skipped conflict this is + // usually the actual reason, and it names the mod to fix rather than leaving + // "needs manual resolution" as the whole story. + foreach (var finding in StaleBuildDetector.Analyze(modIndex.Conflicts)) + Console.WriteLine($" stale mod build: {finding.Describe()}"); + + foreach (var stale in MergeInventoryHygiene.FindStale(AppState.Inventory, AppState.LoadOrder)) + Console.WriteLine($" stale merge record: {stale.Describe()}"); + return summary.Skipped.Count == 0 ? 0 : 2; } @@ -204,6 +214,12 @@ static int RunMcp() return 1; } + // stdout carries JSON-RPC frames from here on, so every notifier line a scan + // or merge emits has to go to stderr instead - see + // HeadlessMergeNotifier.RouteAllOutputToStandardError. Set before the server is + // built, since Core code can emit through the notifier as soon as a tool runs. + HeadlessMergeNotifier.RouteAllOutputToStandardError = true; + var builder = Host.CreateApplicationBuilder(); // stdout is reserved for MCP protocol frames - all logging must go to stderr. diff --git a/WitcherScriptMerger.Tests/Inventory/MergeInventoryHygieneTests.cs b/WitcherScriptMerger.Tests/Inventory/MergeInventoryHygieneTests.cs new file mode 100644 index 0000000..7efee98 --- /dev/null +++ b/WitcherScriptMerger.Tests/Inventory/MergeInventoryHygieneTests.cs @@ -0,0 +1,181 @@ +using System.Collections.Generic; +using System.Linq; +using WitcherScriptMerger.FileIndex; +using WitcherScriptMerger.Inventory; +using Xunit; + +namespace WitcherScriptMerger.Tests.Inventory +{ + // Deliberately narrow. Every rule in MergeInventoryHygiene that concerns a FLAT-file + // record resolves a real path through Merge.GetMergedFile()/GetModFile(), which reads + // Paths.ModsDirectory -> AppState.Settings, whose constructor calls + // Environment.Exit(1) when it can't find a config file - killing the whole test host, + // not just one test (see WitcherScriptMerger.Tests/CLAUDE.md's "AppState.Settings- + // safety constraints"). So these cover exactly the branches that return before any + // path is resolved: the bundle exemption, the null/empty guards, and the pure + // message-formatting. + public class MergeInventoryHygieneTests + { + static Merge BundleMerge(string relativePath = @"scripts\game\thing.ws") => + new Merge { RelativePath = relativePath, BundleName = "blob0.bundle", MergedModName = "mod0000_MergedFiles" }; + + [Fact] + public void HasMergedFile_NullMerge_ReportsPresent() + { + // "Can't prove it's missing" must never read as "it's missing" - a false + // positive here would make HasResolvedConflict re-merge a healthy file. + Assert.True(MergeInventoryHygiene.HasMergedFile(null)); + } + + // Bundle-content records are exempt on purpose: their GetMergedFile() resolves + // under Paths.MergedBundleContent, which is working-directory-relative scratch + // space cleared between runs, so absence there says nothing about whether the + // merge is live - the real artifact is the packed bundle. + [Fact] + public void HasMergedFile_BundleContentRecord_ReportsPresentWithoutTouchingDisk() + { + var merge = BundleMerge(); + + Assert.Equal(Categories.BundleText, merge.Category); + Assert.True(MergeInventoryHygiene.HasMergedFile(merge)); + } + + [Fact] + public void FindStale_NullInventory_ReturnsEmpty() + { + Assert.Empty(MergeInventoryHygiene.FindStale(null)); + } + + [Fact] + public void FindStale_InventoryWithNoRecords_ReturnsEmpty() + { + Assert.Empty(MergeInventoryHygiene.FindStale(new MergeInventory())); + } + + // A bundle record with no source mods exercises FindStale's loop without ever + // reaching GetModFile - it confirms the bundle exemption holds through the + // top-level walk, not just in HasMergedFile itself. + [Fact] + public void FindStale_BundleRecordWithNoMods_ReportsNothing() + { + var inventory = new MergeInventory(); + inventory.Merges.Add(BundleMerge()); + + Assert.Empty(MergeInventoryHygiene.FindStale(inventory)); + } + + [Fact] + public void Describe_SourceModFileMissing_NamesTheFileAndTheMod() + { + var record = new StaleMergeRecord( + new Merge { RelativePath = @"game\r4Game.ws" }, + StaleMergeReason.SourceModFileMissing, + "modGone"); + + var text = record.Describe(); + + Assert.Contains(@"game\r4Game.ws", text); + Assert.Contains("modGone", text); + Assert.Contains("no longer has this file", text); + } + + [Fact] + public void Describe_SourceModDisabled_SaysTheGameWouldNotLoadIt() + { + var record = new StaleMergeRecord( + new Merge { RelativePath = @"game\vehicles\horse\states\exploration.ws" }, + StaleMergeReason.SourceModDisabled, + "modFearlessRoach"); + + var text = record.Describe(); + + Assert.Contains("modFearlessRoach", text); + Assert.Contains("disabled in mods.settings", text); + } + + [Fact] + public void StaleMergeRecord_KeepsTheMergeItDescribes() + { + var merge = new Merge { RelativePath = @"game\npc\npc.ws" }; + var record = new StaleMergeRecord(merge, StaleMergeReason.MergedFileMissing); + + Assert.Same(merge, record.Merge); + Assert.Equal(StaleMergeReason.MergedFileMissing, record.Reason); + Assert.Null(record.ModName); + } + } + + // ModFileIndex.ExcludeDisabledModPaths - the pure function behind the disabled-mod + // scan filter, testable without a real mods.settings or AppState.Settings for the + // same reason (and in the same shape) as BuildIgnoredModNames. + public class ExcludeDisabledModPathsTests + { + static readonly string[] Paths = + { + @"G:\game\Mods\modAlwaysFullExp", + @"G:\game\Mods\modFearlessRoach", + @"G:\game\Mods\modImprovedHorseControls", + }; + + [Fact] + public void ExcludesOnlyTheDisabledMod() + { + var kept = ModFileIndex.ExcludeDisabledModPaths( + Paths, name => name == "modFearlessRoach", out var skipped); + + Assert.Equal(new[] { @"G:\game\Mods\modAlwaysFullExp", @"G:\game\Mods\modImprovedHorseControls" }, kept); + Assert.Equal(new[] { "modFearlessRoach" }, skipped); + } + + // The predicate is asked about the folder NAME - the key mods.settings uses and + // the name the game loads by - never the full path. + [Fact] + public void AsksThePredicateForTheFolderNameNotTheFullPath() + { + var seen = new List(); + ModFileIndex.ExcludeDisabledModPaths(Paths, name => { seen.Add(name); return false; }, out _); + + Assert.Equal(new[] { "modAlwaysFullExp", "modFearlessRoach", "modImprovedHorseControls" }, seen); + } + + [Fact] + public void NothingDisabled_KeepsEveryPathAndSkipsNothing() + { + var kept = ModFileIndex.ExcludeDisabledModPaths(Paths, _ => false, out var skipped); + + Assert.Equal(Paths, kept); + Assert.Empty(skipped); + } + + // A missing/unreadable mods.settings must exclude nobody rather than everybody - + // otherwise a fresh install, or a Linux host with no Documents\The Witcher 3, + // would scan zero mods and silently report zero conflicts. + [Fact] + public void NullPredicate_ExcludesNothing() + { + var kept = ModFileIndex.ExcludeDisabledModPaths(Paths, null, out var skipped); + + Assert.Equal(Paths, kept); + Assert.Empty(skipped); + } + + [Fact] + public void NullPathList_ReturnsEmptyRatherThanThrowing() + { + var kept = ModFileIndex.ExcludeDisabledModPaths(null, _ => true, out var skipped); + + Assert.Empty(kept); + Assert.Empty(skipped); + } + + [Fact] + public void SkippedNamesAreSortedForStableReporting() + { + var paths = new[] { @"M\modZeta", @"M\modAlpha", @"M\modMiddle" }; + + ModFileIndex.ExcludeDisabledModPaths(paths, _ => true, out var skipped); + + Assert.Equal(new[] { "modAlpha", "modMiddle", "modZeta" }, skipped); + } + } +} diff --git a/WitcherScriptMerger.Tests/Tools/StaleBuildDetectorTests.cs b/WitcherScriptMerger.Tests/Tools/StaleBuildDetectorTests.cs new file mode 100644 index 0000000..62f39d7 --- /dev/null +++ b/WitcherScriptMerger.Tests/Tools/StaleBuildDetectorTests.cs @@ -0,0 +1,207 @@ +using System.Linq; +using WitcherScriptMerger.Tools; +using Xunit; + +namespace WitcherScriptMerger.Tests.Tools +{ + // Covers StaleBuildDetector.FindMissingVanillaDeclarations only - the pure, two-string + // core of the pre-flight check. The ModFile-taking Analyze overloads read + // Paths.ScriptsDirectory/ModsDirectory and Paths.RetrieveMergedModName, which force + // AppState.Settings to construct; see WitcherScriptMerger.Tests/CLAUDE.md's + // "AppState.Settings-safety constraints" for why a test must never do that. + public class StaleBuildDetectorTests + { + const string Vanilla = @" +class CR4Game extends CCommonGame +{ + public function GetExpModifier() : float + { + return 1.0f; + } + + event OnHDRChangedEvent() + { + GetGuiManager().OnHDRChanged(); + } + + event OnUserSignedOut() + { + isSignedIn = false; + } +} +"; + + // The real modAlwaysFullExp shape: a whole-file copy from an older game build, so + // the event the newer vanilla added simply isn't there. + const string StaleModCopy = @" +class CR4Game extends CCommonGame +{ + public function GetExpModifier() : float + { + return 1.0f; + } + + event OnUserSignedOut() + { + isSignedIn = false; + } +} +"; + + [Fact] + public void FindMissingVanillaDeclarations_ModCopyFromOlderBuild_ReportsTheMissingDeclaration() + { + var missing = StaleBuildDetector.FindMissingVanillaDeclarations(Vanilla, StaleModCopy); + + Assert.Equal(new[] { "CR4Game::OnHDRChangedEvent" }, missing); + } + + [Fact] + public void FindMissingVanillaDeclarations_ModCopyMatchesVanilla_ReportsNothing() + { + Assert.Empty(StaleBuildDetector.FindMissingVanillaDeclarations(Vanilla, Vanilla)); + } + + // The check is deliberately one-directional. A mod ADDING declarations is the + // entire point of a mod; only vanilla content the mod's copy lacks is evidence + // that the copy predates the installed game build. + [Fact] + public void FindMissingVanillaDeclarations_ModAddsDeclarations_ReportsNothing() + { + var modded = Vanilla.Replace( + " event OnUserSignedOut()", + " public function ModAddedHelper() : bool\r\n {\r\n return true;\r\n }\r\n\r\n event OnUserSignedOut()"); + + Assert.Empty(StaleBuildDetector.FindMissingVanillaDeclarations(Vanilla, modded)); + } + + // Scoped, not bare, names - so a method removed from one class isn't masked by a + // same-named method surviving in another. This is the identity + // FunctionLevelMergeEngine's own invariant reports, which is what lets a + // pre-flight warning and a post-hoc violation name the same thing. + [Fact] + public void FindMissingVanillaDeclarations_SameNameInAnotherClass_DoesNotMaskTheLoss() + { + const string vanilla = @" +class CR4Game extends CCommonGame +{ + event OnHDRChangedEvent() + { + GetGuiManager().OnHDRChanged(); + } +} + +class CR4MapMenu extends CR4MenuBase +{ + event OnHDRChangedEvent() + { + DoSomethingElse(); + } +} +"; + const string mod = @" +class CR4Game extends CCommonGame +{ +} + +class CR4MapMenu extends CR4MenuBase +{ + event OnHDRChangedEvent() + { + DoSomethingElse(); + } +} +"; + var missing = StaleBuildDetector.FindMissingVanillaDeclarations(vanilla, mod); + + Assert.Equal(new[] { "CR4Game::OnHDRChangedEvent" }, missing); + } + + [Fact] + public void FindMissingVanillaDeclarations_MultipleMissing_ReportsAllOrdered() + { + const string vanilla = @" +class CR4MapMenu extends CR4MenuBase +{ + private function SetInitialFilters() + { + } + + event OnFiltersChanged(id : int) + { + } + + public function Keep() + { + } +} +"; + const string mod = @" +class CR4MapMenu extends CR4MenuBase +{ + public function Keep() + { + } +} +"; + var missing = StaleBuildDetector.FindMissingVanillaDeclarations(vanilla, mod); + + Assert.Equal( + new[] { "CR4MapMenu::OnFiltersChanged", "CR4MapMenu::SetInitialFilters" }, + missing.OrderBy(n => n).ToArray()); + } + + // An unscannable file must produce NO finding. Returning an empty set from the + // extractor instead would make every vanilla declaration look missing and turn + // one malformed mod file into a wall of false "older game build" warnings. + [Fact] + public void FindMissingVanillaDeclarations_UnscannableModText_ReportsNothing() + { + Assert.Empty(StaleBuildDetector.FindMissingVanillaDeclarations(Vanilla, "class Broken {")); + } + + [Fact] + public void FindMissingVanillaDeclarations_UnscannableVanillaText_ReportsNothing() + { + Assert.Empty(StaleBuildDetector.FindMissingVanillaDeclarations("class Broken {", Vanilla)); + } + + [Fact] + public void Describe_NamesTheModTheFileAndTheRemedy() + { + var finding = new StaleBuildFinding( + @"game\r4Game.ws", "modAlwaysFullExp", new[] { "CR4Game::OnHDRChangedEvent" }, 42); + + var text = finding.Describe(); + + Assert.Contains("modAlwaysFullExp", text); + Assert.Contains(@"game\r4Game.ws", text); + Assert.Contains("CR4Game::OnHDRChangedEvent", text); + Assert.Contains("older game version", text); + Assert.Contains("disable it", text); + + // The check runs before any merge, so it must not assert an outcome it can't + // know - a small drift often merges cleanly. It reports the drift and the + // usual consequence, never a verdict on this specific conflict. + Assert.DoesNotContain("can't be auto-merged", text); + Assert.Contains("usual reason", text); + } + + // Long lists get truncated so a badly out-of-date mod doesn't print hundreds of + // names, but the true total still has to be visible. + [Fact] + public void Describe_ManyMissingDeclarations_TruncatesNamesButKeepsTheCount() + { + var names = Enumerable.Range(0, StaleBuildDetector.MaxNamesInDescription + 4) + .Select(i => $"CR4Game::Fn{i}") + .ToArray(); + var finding = new StaleBuildFinding(@"game\r4Game.ws", "modStale", names, 99); + + var text = finding.Describe(); + + Assert.Contains($"missing {names.Length} of the 99 declaration(s)", text); + Assert.Contains("+4 more", text); + Assert.DoesNotContain($"CR4Game::Fn{names.Length - 1}", text); + } + } +} diff --git a/WitcherScriptMerger/App.config b/WitcherScriptMerger/App.config index b5b0ce0..c7844f7 100644 --- a/WitcherScriptMerger/App.config +++ b/WitcherScriptMerger/App.config @@ -10,6 +10,15 @@ CheckXmlFiles Whether to check for mod conflicts in .xml files CheckBundleContents Whether to check for mod conflicts in bundle file contents IgnoreModNames Which mod folders to ignore (separated by commas) +MergeDisabledMods Whether to scan mod folders that mods.settings marks Enabled=0. + Defaults to false: the game never loads a disabled mod, so it can't + really conflict with anything, and counting it anyway can keep a + conflict permanently unmergeable (a disabled mod shipping a whole-file + copy from an older game build trips the vanilla-declaration invariant + on every run). Set to true to pre-merge mods that are staged but not + switched on yet. A mod absent from mods.settings counts as enabled - + only an explicit Enabled=0 excludes it. + AdditionalVanillaDlcFolderNames Extra vanilla DLC/expansion folder names (under GameDirectory\DLC) to treat as a 3-way merge's vanilla baseline, in addition to the @@ -46,6 +55,7 @@ WccLitePath Where wcc_lite.exe is located + diff --git a/WitcherScriptMerger/Program.cs b/WitcherScriptMerger/Program.cs index a76a2a4..dc281fa 100644 --- a/WitcherScriptMerger/Program.cs +++ b/WitcherScriptMerger/Program.cs @@ -15,6 +15,7 @@ using WitcherScriptMerger.Inventory; using WitcherScriptMerger.LoadOrder; using WitcherScriptMerger.Mcp; +using WitcherScriptMerger.Tools; namespace WitcherScriptMerger { @@ -246,6 +247,15 @@ static int RunCli(string[] args) foreach (var decision in summary.FunctionLevelDecisions) Console.WriteLine($" function-level: {decision}"); + // See the identical block in WitcherScriptMerger.Headless/Program.cs - both CLI + // entry points report the same diagnostics, since either can be the one a user + // (or the Vortex extension) actually runs. + foreach (var finding in StaleBuildDetector.Analyze(modIndex.Conflicts)) + Console.WriteLine($" stale mod build: {finding.Describe()}"); + + foreach (var stale in MergeInventoryHygiene.FindStale(Inventory, LoadOrder)) + Console.WriteLine($" stale merge record: {stale.Describe()}"); + return summary.Skipped.Count == 0 ? 0 : 2; } @@ -276,6 +286,12 @@ static int RunMcp() return 1; } + // stdout carries JSON-RPC frames from here on, so every notifier line a scan + // or merge emits has to go to stderr instead - see + // HeadlessMergeNotifier.RouteAllOutputToStandardError. Set before the server is + // built, since Core code can emit through the notifier as soon as a tool runs. + HeadlessMergeNotifier.RouteAllOutputToStandardError = true; + var builder = Host.CreateApplicationBuilder(); // stdout is reserved for MCP protocol frames - all logging must go to stderr. From 628432235bda1a96659d2040f18f68f950ea766d Mon Sep 17 00:00:00 2001 From: Chris Knight Date: Tue, 25 Aug 2026 23:09:32 -0400 Subject: [PATCH 2/2] Document the stale-build pre-flight, inventory hygiene and disabled-mod skip Core's CLAUDE.md gains three sections ("Disabled mods are excluded from the conflict scan", "Stale-build pre-flight", "Inventory hygiene"), the folder map and MCP tool return shapes are updated, and the IMergeNotifier section now covers why MCP mode has to force notifier output to stderr. docs/bugs/stale-mod-builds-and-phantom-merge-records.md records the episode in the same shape as function-level-merge-gap-handling.md: what recurred, the three real files and their blocking mods, why each gap kept it recurring, and the before/after numbers from the live install. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01N6y39x9F3SKcS4hhpw85Yi --- WitcherScriptMerger.Core/CLAUDE.md | 127 +++++++++++++++++- WitcherScriptMerger.Tests/CLAUDE.md | 24 ++++ ...le-mod-builds-and-phantom-merge-records.md | 120 +++++++++++++++++ 3 files changed, 266 insertions(+), 5 deletions(-) create mode 100644 docs/bugs/stale-mod-builds-and-phantom-merge-records.md diff --git a/WitcherScriptMerger.Core/CLAUDE.md b/WitcherScriptMerger.Core/CLAUDE.md index 72b7a4e..34ba565 100644 --- a/WitcherScriptMerger.Core/CLAUDE.md +++ b/WitcherScriptMerger.Core/CLAUDE.md @@ -19,7 +19,8 @@ still external dependencies rather than an in-process replacement. - `Inventory/` — core merge domain + persistence: `FileMerger.cs` (headless orchestration plus TreeNode-free interactive orchestration — see "FileMerger: interactive vs. headless split" below), `Merge.cs`, `MergeInventory.cs`, `FileHash.cs`, - `MergeProgressInfo.cs`. + `MergeProgressInfo.cs`, `MergeInventoryHygiene.cs` (stale-record detection — see + "Inventory hygiene" below). - `LoadOrder/` — mod load-order logic: `CustomLoadOrder.cs`, `LoadOrderComparer.cs`, `LoadOrderValidator.cs`, `ModLoadSetting.cs`. - `Tools/` — wrappers that shell out to bundled external executables, plus the sole @@ -29,7 +30,9 @@ still external dependencies rather than an in-process replacement. default associated app" helper; the only call site is `DiffPlexMergeEngine.MergeHeadless`, opening a genuine conflict's marker sidecar), `ScriptUnitExtractor.cs`/`UnitAligner.cs`/`FunctionLevelMergeEngine.cs` (the - function-level merge fallback — see "Function-level merge engine" below). + function-level merge fallback — see "Function-level merge engine" below), + `StaleBuildDetector.cs` (the pre-flight counterpart to that engine's + vanilla-declaration invariant — see "Stale-build pre-flight" below). - `Cli/` — `MergeOperations.cs`: the scan-then-merge sequence shared by both hosts' `merge` CLI verb and by the MCP tools (see "CLI & MCP orchestration" below). - `Mcp/` — `WsmMcpTools.cs`: the MCP server's tool implementations (see below and @@ -93,7 +96,16 @@ non-destructive for this specific prompt" — added for `LoadOrderValidator`'s the one destructive/permanent choice there). `HeadlessMergeNotifier` writes to the console and returns a fixed, non-destructive default per button set (don't overwrite, don't use a still-conflicting merge name, don't continue past a failure) unless the -caller overrides it via `defaultResult`. The WinForms host's `MainForm` implements this +caller overrides it via `defaultResult`. Its static `RouteAllOutputToStandardError` flag, +set by both hosts' `mcp` verb before the server starts, forces **every** line to stderr: +in MCP mode stdout carries the JSON-RPC frame stream, and a notifier line printed there +lands mid-stream and corrupts the transport — the client sees unparseable JSON rather +than a message from WSM. Ordinarily only `Error`/`Warning`/`Exclamation` route to stderr, +which meant any default-icon `ShowMessage` a scan can reach corrupted the stream: +`ModFileIndex.BuildAsync`'s "Can't find any mods in the Mods directory.", and the +disabled-mod skip notice (see below). Caught by a real `scan_conflicts` round-trip +returning a payload the client couldn't parse, with the notice spliced between two +protocol frames. CLI mode is unaffected — the flag stays `false` there. The WinForms host's `MainForm` implements this interface too, translating the neutral types to/from real WinForms calls — see `WitcherScriptMerger/CLAUDE.md` for that translation and the one behavior change it introduced. @@ -241,6 +253,108 @@ pure function behind `GetIgnoredModNames()`, split out so it's unit-testable wit touching `AppState.Settings` — see `WitcherScriptMerger.Tests/CLAUDE.md`'s "`AppState.Settings`-safety constraints" and `FileIndex/ModFileIndexTests.cs`. +## Disabled mods are excluded from the conflict scan + +`ModFileIndex.BuildAsync` also drops any mod folder that `mods.settings` marks +`Enabled=0`, via `ExcludeDisabledMods` → `CustomLoadOrder.IsModDisabledByName`. + +A scan is a filesystem glob over `Paths.ModsDirectory`, so before this a deployed-but- +disabled mod looked exactly like an active one and its files counted as full conflict +participants. That is not a cosmetic over-count — **a disabled mod can make a conflict +permanently unmergeable**. Observed live: a disabled `modFearlessRoach` ships a +pre-next-gen whole-file copy of `game\vehicles\horse\states\exploration.ws` (missing +`CheckVector`/`DoHorseKick`/`OnHorseKick` plus member declarations), which trips +`FunctionLevelMergeEngine`'s vanilla-declaration invariant, so every single run reported +"needs manual resolution" for a file the game was never going to load that mod's version +of anyway. On that install the change took the conflict count from 44 to 43, with the +remaining 43 all resolving cleanly. + +**Only an explicit `Enabled=0` excludes a mod.** `IsModDisabledByName` returns `false` +for a mod absent from `mods.settings` entirely, which is the correct reading — the game +appends unknown mod folders on next launch, enabled. A missing or unreadable +`mods.settings` likewise disables nobody (`CustomLoadOrder.Refresh` leaves `Mods` empty), +so this can never make a scan miss conflicts on a fresh install or on a Linux host with +no `Documents\The Witcher 3` at all. + +The `MergeDisabledMods` App.config setting opts out (scan every deployed mod regardless), +the useful case being pre-merging a mod that's staged but not switched on yet. It is +deliberately named for the opt-**out** so that its absence from an older `App.config` +yields the new, wanted behavior: `AppSettings.Get` returns `false` for a key that +isn't there. Skipped folders are reported through `AppState.Notifier` and exposed as +`ModFileIndex.DisabledModsSkipped` (and the MCP `merge_conflicts` tool's +`disabledModsSkipped`) — never silent, since a conflict disappearing from a scan should +be visible rather than inferred from a changed count. + +`ModFileIndex.ExcludeDisabledModPaths(paths, isModDisabled, out skipped)` is the pure +function behind it, split out to be unit-testable without `AppState.Settings` or a real +`mods.settings` — same shape and same reason as `BuildIgnoredModNames` above. + +## Stale-build pre-flight (`Tools/StaleBuildDetector.cs`) + +The pre-flight counterpart to `ValidateWholeFileMergeOutput`'s vanilla-declaration +invariant (see "Function-level merge engine" above). That invariant is the safety net — +it fires *after* a merge has produced output that would have dropped vanilla +declarations, and its message correctly guesses the cause ("...usually means that mod +ships a whole-file copy taken from an older game build"). But by then the user is looking +at a "Skipped — needs manual resolution" line, with the actionable fact (which mod is out +of date, and that the remedy is to update or disable *that mod* rather than hand-merge a +2500-line file) buried inside a sentence about a DiffPlex bug. + +`FindMissingVanillaDeclarations(vanillaText, modText)` runs the same comparison up front, +straight off the files with no merge involved: every `ScriptUnitExtractor` scoped name the +installed vanilla file has that a given mod's copy does not. Using the extractor's own +`ScopedName` identity is what makes a pre-flight warning and a post-hoc violation name the +same thing. An unscannable file on either side yields **no** finding — returning an empty +set instead would make every vanilla declaration look missing and turn one malformed mod +file into a wall of false warnings. + +**A diagnostic, never a gate.** Nothing here changes what does or doesn't merge: a mod +*may* legitimately delete a vanilla function, and the check runs before any merge so it +cannot know whether this particular conflict will actually fail. The message says so — +it reports the drift and its usual consequence rather than asserting an outcome. That +distinction is load-bearing: on a real install one mod was missing 1 of 224 declarations +and its conflict auto-solved every time, while another missing 13 of 224 reliably tripped +the invariant. + +Surfaced via both hosts' `merge` CLI output (`stale mod build:` lines), the MCP +`scan_conflicts` tool's per-conflict `staleBuildWarnings`, and `merge_conflicts`'s +top-level `staleBuildWarnings`. Only `.ws` conflicts are examined — `ScriptUnitExtractor` +is WitcherScript-specific, exactly as the function-level rescue itself is gated — and the +merged-mod folder is skipped, since it routinely appears among a conflict's sources once +a file has been merged once and an "older game build" verdict on this tool's own output +would be meaningless. + +## Inventory hygiene (`Inventory/MergeInventoryHygiene.cs`) + +Three staleness rules the WinForms GUI has always applied in +`MainForm.RefreshMergeTree()` — merged file missing, source mod file missing, source mod +disabled — lifted out as pure, promptless predicates so the headless CLI, the MCP tools +and the Vortex extension see the same staleness the GUI does. + +**`MergeInventory.HasResolvedConflict` now requires the merged output to exist** +(`HasMergedFile`). Without that check the record was self-certifying: every hash it +verifies belongs to a *source* mod, all of which are present and unchanged when it's the +*output* that has been deleted, so it answered "resolved" forever and nothing ever +re-merged. Observed live for `game\vehicles\horse\states\exploration.ws` — record +present, merged file absent, `alreadyResolved: true` on every scan, and the game +therefore loading exactly one of the two conflicting mods with nothing indicating a +problem. + +Bundle-content records are exempt from that existence check: their `GetMergedFile()` +resolves under `Paths.MergedBundleContent`, which is working-directory-relative scratch +space cleared between runs, so absence there says nothing about whether the merge is live +— the real artifact is the packed bundle. Only flat files have a stable, absolute output +path under the mods directory that "missing" is meaningful for, and flat files are the +only category either headless host supports anyway. + +**Findings, not actions.** Deciding what to *do* about a stale record stays the caller's: +the GUI asks the user (`ConfirmPruneMissingMergeFile` and friends), headless callers +report it (`stale merge record:` CLI lines, `list_merges`'s `mergedFileExists` / +`staleWarnings`). That split is deliberate rather than lazy — the GUI's three prompts all +pass no `defaultResult`, so `HeadlessMergeNotifier` would answer its generic +`YesNo => No` to every one of them and prune nothing, silently. Same defect shape as the +`ConfirmOutputOverwrite` finding in `docs/bugs/function-level-merge-gap-handling.md`. + ## CLI & MCP orchestration (`Cli/`, `Mcp/`) `Cli/MergeOperations.cs` is the scan-then-merge sequence shared by both hosts' `merge` @@ -264,8 +378,11 @@ itself, since a file that's already been merged once has its own merged-mod fold re-enter `conflict.Mods` as if it were a source. `Mcp/WsmMcpTools.cs` (`[McpServerToolType]` static class) exposes four tools — -`scan_conflicts`, `merge_conflicts` (optional `relativePaths`/`orderOverrides`/`dryRun`; -returns `{merged, skipped, unmatched, dryRun}`), `get_status`, `list_merges` — all +`scan_conflicts` (each conflict additionally carries `staleBuildWarnings`), +`merge_conflicts` (optional `relativePaths`/`orderOverrides`/`dryRun`/`overwrite`; +returns `{merged, skipped, unmatched, dryRun, overwrite, functionLevelDecisions, +staleBuildWarnings, disabledModsSkipped}`), `get_status`, `list_merges` (each record +additionally carries `mergedFileExists` and `staleWarnings`) — all reusing `MergeOperations` and the same `IMergeNotifier` machinery as the CLI verb. `get_status` reports `textMergeDependenciesValid` and `bundleDependenciesValid` as two independent fields (plus a combined `dependenciesValid`, kept for existing callers that diff --git a/WitcherScriptMerger.Tests/CLAUDE.md b/WitcherScriptMerger.Tests/CLAUDE.md index c5d6b88..3074bc2 100644 --- a/WitcherScriptMerger.Tests/CLAUDE.md +++ b/WitcherScriptMerger.Tests/CLAUDE.md @@ -76,6 +76,30 @@ does. fall through correctly), malformed/truncated/empty XML degrading to `null` rather than throwing, null/blank inputs, and that `VortexSidecarFileName` still matches the name Vortex hardcodes — see Core's `CLAUDE.md`'s "Vortex-managed sidecar config" section. +- `Tools/StaleBuildDetectorTests.cs` — `StaleBuildDetector.FindMissingVanillaDeclarations`, + the pure two-string core of the stale-build pre-flight check (see Core's `CLAUDE.md`'s + "Stale-build pre-flight"): a mod copy from an older build reporting the declaration it + lacks, an identical copy and a mod that only *adds* declarations both reporting nothing + (the check is deliberately one-directional), scoped names keeping a same-named method in + another class from masking a real loss, and unscannable text on *either* side yielding no + finding rather than a wall of false positives. Also `StaleBuildFinding.Describe`'s + wording, including a regression assertion that it does **not** claim the conflict "can't + be auto-merged" — the check runs before any merge and can't know that. The + `ModFile`-taking `Analyze` overloads are deliberately not covered: they read + `Paths.ScriptsDirectory`/`ModsDirectory` and `Paths.RetrieveMergedModName`, which force + `AppState.Settings` to construct (see below). +- `Inventory/MergeInventoryHygieneTests.cs` — `MergeInventoryHygiene` and + `ModFileIndex.ExcludeDisabledModPaths`. Deliberately narrow on the hygiene side: every + rule concerning a *flat-file* record resolves a real path through + `Merge.GetMergedFile()`/`GetModFile()` → `Paths.ModsDirectory` → `AppState.Settings`, so + coverage is limited to the branches that return before any path is resolved — the + bundle-content exemption, the null/empty guards, and the pure message formatting. + `ExcludeDisabledModPaths` (the pure function behind the disabled-mod scan filter — see + Core's `CLAUDE.md`'s "Disabled mods are excluded from the conflict scan") is covered + fully: only the disabled mod excluded, the predicate asked for the folder *name* rather + than the full path, stable sorting of skipped names, and — the important one — a `null` + predicate or `null` path list excluding *nothing* rather than everything, so an + unreadable `mods.settings` can never make a scan silently report zero conflicts. - `LiveInstall.cs` — see "Live-install cross-check tests" below. ## `AppState.Settings`-safety constraints diff --git a/docs/bugs/stale-mod-builds-and-phantom-merge-records.md b/docs/bugs/stale-mod-builds-and-phantom-merge-records.md new file mode 100644 index 0000000..ba5ec9d --- /dev/null +++ b/docs/bugs/stale-mod-builds-and-phantom-merge-records.md @@ -0,0 +1,120 @@ +# "Manual merges are needed" that never stops recurring + +**Status:** fixed (this batch) +**Found:** 2026-08-25, WSM 0.7.0 (`WitcherScriptMerger.Headless mcp` → `scan_conflicts` / +`merge_conflicts`), against a live 350-mod, game-build-4.04 install +**Severity:** medium — no data loss, but three conflicts were reported as needing manual +resolution on every single run, one of them permanently invisible to any headless caller +**Component:** `WitcherScriptMerger.Core/FileIndex/ModFileIndex.cs`, +`Inventory/MergeInventory.cs`, `HeadlessMergeNotifier.cs` + +## Summary + +A merge run reported three files as "needs manual resolution", every time, with no way to +make it stop. The merge engine was **behaving correctly** in all three cases — it was +refusing to reproduce real damage — but nothing said which mod to fix, and two separate +gaps guaranteed the same three files came back on the next run. + +| File | Blocking mod | Enabled? | State of merged output | +|---|---|---|---| +| `game\vehicles\horse\states\exploration.ws` | `modFearlessRoach` | **no (`Enabled=0`)** | **none at all** | +| `game\gui\menus\mapMenu.ws` | `modFastTravelFromAnywhere` | yes | 4 days stale | +| `game\r4Game.ws` | `modAlwaysFullExp` | yes | 4 days stale | + +All 41 other conflicts in the same run merged cleanly. The three that didn't were exactly +the three whose source mods ship a whole-file copy of a vanilla script taken from an +**older game build** — the `ValidateWholeFileMergeOutput` case documented in +`WitcherScriptMerger.Core/CLAUDE.md`'s "Function-level merge engine". Verbatim from a dry +run: + +``` +[Skipped] Skipped modalchemyrequiresmeditation + modAlwaysFullExp: the whole-file merge +silently corrupted content ('CR4Game::OnHDRChangedEvent' is declared in the vanilla file +and kept by modalchemyrequiresmeditation, but is missing from the merged output (lost) - +modAlwaysFullExp has no copy of it, which usually means that mod ships a whole-file copy +taken from an older game build) and the function-level fallback declined. Needs manual +resolution - open the source mod files directly to compare and resolve. +``` + +That diagnosis is correct, and the invariant is right to decline: taking the stale mod's +side would delete vanilla code the game and the other contributing mod still call. The +already-deployed `r4Game.ws` (merged by a pre-invariant engine build) confirmed it — +`OnHDRChangedEvent` was already missing from the live merged file. + +## Why it kept recurring + +### 1. The scan ignored `mods.settings` + +`ModFileIndex.BuildAsync` globbed `Mods\mod*` off disk, filtered only by +`IgnoreModNames`. `modFearlessRoach` is deployed but `Enabled=0` — the game never loads +it, so it cannot really conflict with anything. WSM counted it anyway, tried to merge its +1.3x-era `exploration.ws` (2447 lines vs vanilla's 2734, missing `CheckVector` / +`DoHorseKick` / `OnHorseKick` and five member declarations), failed the invariant, and +reported "needs manual resolution" — forever. One of the three was a pure phantom. + +Fixed: `ExcludeDisabledMods` (see Core's `CLAUDE.md`, "Disabled mods are excluded from +the conflict scan"), opt-out via `MergeDisabledMods`. + +### 2. `HasResolvedConflict` never checked that the merged output exists + +The record for `exploration.ws` claimed the conflict was resolved while no merged file +existed at all. Every hash it verifies belongs to a *source* mod — all present and +unchanged — so the record was self-certifying: `alreadyResolved: true` on every scan, +never re-merged, and the game silently loading exactly one of the two conflicting mods. + +The WinForms GUI already refused to trust such a record (`MainForm.RefreshMergeTree` → +`ConfirmPruneMissingMergeFile`), along with two sibling rules for a missing or disabled +source mod. **All three were GUI-only**, so the CLI, the MCP tools and the Vortex +extension never saw them. + +Fixed: `MergeInventoryHygiene` + the existence check in `HasResolvedConflict` (see Core's +`CLAUDE.md`, "Inventory hygiene"). Note the GUI's prompts all pass no `defaultResult`, so +hoisting that code as-is would have had `HeadlessMergeNotifier` answer its generic +`YesNo → No` and prune nothing, silently — the same defect shape as the +`ConfirmOutputOverwrite` finding in `function-level-merge-gap-handling.md`. The headless +rules therefore *report*; only the GUI acts. + +### 3. Nothing named the mod to fix + +"Needs manual resolution" sent the user toward hand-merging a 2500-line script. The real +remedy was to update or drop one mod — and each mod's *actual* intended change turned out +to be tiny: `modAlwaysFullExp` is **2 lines** (`expModifier = 1.0f;` plus one buff call) +wrapped in a copy missing 68 lines of current vanilla; `modFastTravelFromAnywhere` is 15 +lines, missing 28. + +Fixed: `Tools/StaleBuildDetector.cs`, a pre-flight run of the same comparison the +invariant makes post-hoc, surfaced in `scan_conflicts`, `merge_conflicts` and both hosts' +CLI output. It is a diagnostic, not a gate — see Core's `CLAUDE.md`, "Stale-build +pre-flight", for why its message deliberately stops short of predicting failure. + +## Found while fixing: notifier output corrupts the MCP transport + +`HeadlessMergeNotifier.Write` routed only `Error`/`Warning`/`Exclamation` to stderr; +everything else went to **stdout**, which in `mcp` mode carries the JSON-RPC frame stream. +The new disabled-mod notice (default icon) duly appeared spliced between two protocol +frames in a real `scan_conflicts` round-trip, leaving the client with unparseable JSON. + +Pre-existing, not introduced here — `ModFileIndex.BuildAsync`'s "Can't find any mods in +the Mods directory." had the same shape. Fixed at the root with +`HeadlessMergeNotifier.RouteAllOutputToStandardError`, set by both hosts' `mcp` verb. + +## Verification + +Against the live install, before and after: + +- **Before:** 44 conflicts, 3 unmergeable, 1 inventory record with no output file. +- **After:** 43 conflicts (the disabled-mod phantom gone), **43/43 resolved, 0 skipped**. +- Re-running with `WSM_MergeDisabledMods=true` brings the phantom back *and* prints the + stale-build warning naming `modFearlessRoach` and 13 of the 224 declarations it lacks — + confirming both features on one run. +- The two enabled mods' intended changes were ported onto current vanilla (backups in a + local, non-committed folder), after which both files merged with zero warnings and zero + function-level decisions. Verified: no vanilla declaration lost, both mods' changes + present, brace balance identical to vanilla. + +A static pass over all 44 conflicts (136 mod-copy comparisons) found the stale-build +signature in exactly 3 — the same 3 the engine went on to decline. No false positives, no +misses, at the coarse granularity used for that sweep; the shipped detector uses +`ScriptUnitExtractor`'s full unit set and is correspondingly more sensitive (it also flags +a 1-of-224 drift in `modImprovedHorseControls`, whose conflict auto-solves fine — which is +precisely why the warning reports drift rather than predicting failure).