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.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/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/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. 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).