From 4c0700acd1c15710b5cd22c6ebc10378306ebd73 Mon Sep 17 00:00:00 2001 From: Chris Knight Date: Fri, 7 Aug 2026 18:21:56 -0400 Subject: [PATCH 1/3] Retire KDiff3; make DiffPlexMergeEngine the sole merge engine KDiff3 (Tools/KDiff3.cs, Tools/KDiff3MergeEngine.cs) and the IMergeEngine interface that used to sit in front of it are deleted. DiffPlexMergeEngine is now the only text-merge engine, called directly by FileMerger - no more engine-selection step at startup, no more MergeEngine/KDiff3Path/ ReviewEachMerge/ShowPathsInKDiff3 App.config settings. User-facing behavior change: a genuine conflict that needs manual resolution no longer opens KDiff3's merge-editor window. Instead it writes a git/diff3-style conflict-marker sidecar file and opens it in the OS's default editor for that file type (Tools/FileOpener.cs, Core-side, Process.Start with UseShellExecute=true) - in both the GUI's interactive path and the CLI/MCP headless path, since they're now the same code path underneath. A dry-run merge_conflicts call still writes the sidecar for a would-be conflict but does not open it, since a preview must not have that kind of side effect. This is a deliberate tradeoff, not a strict improvement: DiffPlexMergeEngine has a measured non-zero failure rate on dense multi-edit conflicts and no vanilla-less 2-way fallback, both of which KDiff3 handled. See docs/decisions/kdiff3-retirement.md for the full rationale and for the empirical KDiff3 process-behavior findings (window-title polling, the 250ms-poll-interval constraint, failed window-suppression attempts, unverified focus restoration) preserved now that the motivating code is gone - previously this only lived in the local, gitignored HANDOFF.md. CLAUDE.md, CONTRIBUTING.md, README.md updated throughout to reflect KDiff3 being gone. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01GXAuGMLB44T5Zv5o5ZzKah --- CLAUDE.md | 63 ++-- CONTRIBUTING.md | 8 +- README.md | 4 +- WitcherScriptMerger.Core/AppState.cs | 5 - .../Cli/MergeOperations.cs | 5 +- .../Inventory/FileMerger.cs | 49 ++- WitcherScriptMerger.Core/Mcp/WsmMcpTools.cs | 9 +- WitcherScriptMerger.Core/Paths.cs | 16 +- .../Tools/DiffPlexMergeEngine.cs | 171 ++++++---- .../Tools/FileEncoding.cs | 27 +- WitcherScriptMerger.Core/Tools/FileOpener.cs | 57 ++++ .../Tools/IMergeEngine.cs | 49 --- .../Tools/DiffPlexMergeEngineTests.cs | 131 +++++++- .../Tools/KDiff3CrossCheckTests.cs | 30 +- WitcherScriptMerger/App.config | 13 - .../Forms/DependencyForm.Designer.cs | 115 +------ WitcherScriptMerger/Forms/DependencyForm.cs | 16 +- .../Forms/OptionsForm.Designer.cs | 56 +--- WitcherScriptMerger/Forms/OptionsForm.cs | 4 - .../Inventory/InteractiveMergeRunner.cs | 2 +- WitcherScriptMerger/Program.cs | 19 +- WitcherScriptMerger/Tools/KDiff3.cs | 302 ------------------ .../Tools/KDiff3MergeEngine.cs | 39 --- docs/decisions/kdiff3-retirement.md | 291 +++++++++++++++++ docs/vortex-extension-design.md | 9 + 25 files changed, 737 insertions(+), 753 deletions(-) create mode 100644 WitcherScriptMerger.Core/Tools/FileOpener.cs delete mode 100644 WitcherScriptMerger.Core/Tools/IMergeEngine.cs delete mode 100644 WitcherScriptMerger/Tools/KDiff3.cs delete mode 100644 WitcherScriptMerger/Tools/KDiff3MergeEngine.cs create mode 100644 docs/decisions/kdiff3-retirement.md diff --git a/CLAUDE.md b/CLAUDE.md index 2ff8a21..c16bea4 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -4,31 +4,31 @@ This file provides guidance to Claude Code (claude.ai/code) when working with co ## Project overview -Script Merger for The Witcher 3 — a Windows desktop tool (WinForms, not WPF) that detects and merges conflicting mod script files. It scans a mod folder, finds `.ws`/`.xml` files (including inside `.bundle` packages) that multiple mods modify, and drives a 3-way merge (vanilla + mod1 + mod2) via the external tool KDiff3. `.bundle` package contents are unpacked with QuickBMS and repacked with wcc_lite. +Script Merger for The Witcher 3 — a Windows desktop tool (WinForms, not WPF) that detects and merges conflicting mod script files. It scans a mod folder, finds `.ws`/`.xml` files (including inside `.bundle` packages) that multiple mods modify, and drives a 3-way merge (vanilla + mod1 + mod2) via an in-process DiffPlex-based merge engine (`Tools/DiffPlexMergeEngine.cs`, Core). `.bundle` package contents are unpacked with QuickBMS and repacked with wcc_lite. KDiff3 was formerly used for text merges instead — it was retired; see `docs/decisions/kdiff3-retirement.md` for why and for the empirical KDiff3 process-behavior findings preserved there now that the code is gone. This is a fork of the upstream `AnotherSymbiote/WitcherScriptMerger` repo (still the `origin` remote — no separate fork exists yet), currently mid-modernization. See `HANDOFF.md` at the repo root for the full rationale behind the fork and detailed gotchas hit during the .NET modernization — read it before picking up follow-on work in this repo. Of its original list of open goals, whitespace/diff-noise and a CLI mode (see "CLI mode" below) are done; dependency-packaging/licensing decisions are still open. An MCP server mode (see "MCP mode" below) was added afterward, beyond that original list, to let an MCP client (e.g. Claude Code) drive merges directly instead of only through the CLI. ## Build & run - Build: `dotnet build WitcherScriptMerger.sln` from the repo root. -- Run (GUI, no args): launch the built `WitcherScriptMerger.exe`, or `dotnet run --project WitcherScriptMerger/WitcherScriptMerger.csproj`. At startup the app validates `KDiff3Path`/`QuickBmsPath`/`QuickBmsPluginPath`/`WccLitePath` from `App.config` (`Paths.ValidateDependencyPaths` in `WitcherScriptMerger/Paths.cs`) and shows a blocking `DependencyForm` if any are missing — the external binaries (KDiff3, QuickBMS, wcc_lite) are **not** in source control (see "External tool dependencies" below), so a fresh checkout won't run end-to-end without sourcing them separately. +- Run (GUI, no args): launch the built `WitcherScriptMerger.exe`, or `dotnet run --project WitcherScriptMerger/WitcherScriptMerger.csproj`. At startup the app validates `QuickBmsPath`/`QuickBmsPluginPath`/`WccLitePath` from `App.config` (`Paths.ValidateDependencyPaths` in `WitcherScriptMerger.Core/Paths.cs`) and shows a blocking `DependencyForm` if any are missing — the external binaries (QuickBMS, wcc_lite) are **not** in source control (see "External tool dependencies" below), so a fresh checkout won't run end-to-end without sourcing them separately. (KDiff3 used to be a third dependency validated here; it's been retired — see `docs/decisions/kdiff3-retirement.md`.) - Run (CLI, headless): `WitcherScriptMerger.exe merge [--order-file ]` — see "CLI mode" below. Any arguments at all route to the CLI path instead of the GUI. - Run (MCP server): `WitcherScriptMerger.exe mcp` — see "MCP mode" below. Speaks MCP over stdio; not meant to be run interactively from a terminal. - Three projects, one `.sln`: `WitcherScriptMerger.Core` (WinForms-free class library, `net10.0`), `WitcherScriptMerger` (the WinForms host — GUI + CLI + MCP entry points, `net10.0-windows7.0`, references Core), and `WitcherScriptMerger.Tests` (xunit, `net10.0`, references Core only). See "Architecture" below for what lives where. ### Tests -`WitcherScriptMerger.Tests` (xunit) covers `WitcherScriptMerger.Core` — run with `dotnet test WitcherScriptMerger.Tests/WitcherScriptMerger.Tests.csproj` (or `dotnet test WitcherScriptMerger.sln`). It does **not** reference the host project (no WinForms, no `Tools/KDiff3.cs`), so `KDiff3MergeEngine`/`KDiff3.cs` still have no automated coverage — only `DiffPlexMergeEngine` and other Core-side logic (`Hasher`, `FileEncoding`) do. +`WitcherScriptMerger.Tests` (xunit) covers `WitcherScriptMerger.Core` — run with `dotnet test WitcherScriptMerger.Tests/WitcherScriptMerger.Tests.csproj` (or `dotnet test WitcherScriptMerger.sln`). It does **not** reference the host project (no WinForms) - `DiffPlexMergeEngine` (the sole text-merge engine; KDiff3 was retired, see `docs/decisions/kdiff3-retirement.md`) and other Core-side logic (`Hasher`, `FileEncoding`) are covered directly. - Tests never construct `FileMerger.MergeSource` via `MergeSource.FromFlatFile`/`FromBundle` (both call `ModFile.GetModNameFromPath` → `Paths.ModsDirectory` → `AppState.Settings`) or otherwise force `AppState.Settings` to construct outside a real GUI/CLI/MCP entry point: `AppSettings`'s constructor calls `Environment.Exit(1)` if it can't find a config file next to `Assembly.GetEntryAssembly().Location`, and in a `dotnet test` host (`testhost.dll`, no matching `.config`) that kills the entire test process, not just one test. `AppState.Settings` is a lazy property specifically so that merely touching `AppState.Notifier` (which Core code — e.g. `DiffPlexMergeEngine`'s headless skip/guard messages — legitimately does on its own) doesn't also force `Settings` to construct; see `AppState.cs` and "Startup flow" below. `Paths.cs`'s own properties (`ScriptsDirectory`/`ModsDirectory`/`IsScriptsDirectoryDerived`/`IsModsDirectoryDerived`) read `AppState.Settings.Get(...)` on every access rather than caching the result via a static field initializer, for the identical reason one layer further out: a field initializer there would've forced `Settings` to construct merely from touching an unrelated static member of `Paths` (e.g. `GetRelativePath`), via C#'s beforefieldinit semantics, which would've silently undermined `AppState.Settings`' own laziness. `Tools/DiffPlexMergeEngine.GetConflictMarkerPath` reads only the compile-time-literal `Paths.DiffPlexConflictsDirectory` const, which never triggers `Paths`' type initializer at all, so it's safe to call from tests unconditionally. Tests that need a `FileMerger.MergeSource` build it directly via object-initializer syntax instead (its fields are all public). -- A few tests optionally cross-check against a real Witcher 3 + WitcherScriptMerger install (a live `MergeInventory.xml`'s recorded hashes, or the real `KDiff3.exe` binary for an auto-solvable-only A/B check against `DiffPlexMergeEngine`) via `WitcherScriptMerger.Tests/LiveInstall.cs`, gated entirely on the `WSM_TEST_GAME_DIR` environment variable (unset by default) — never a hardcoded or scanned path, per this repo's "scrub machine-specific paths" rule (see `CONTRIBUTING.md`). They silently no-op when it's unset. -- For anything not covered by the test project — especially further hash-output, `MergeInventory.xml` schema, or KDiff3-invocation changes — the precedent set in `HANDOFF.md` still applies: a disposable, non-committed `dotnet new console` scratch app, exercising synthetic edge cases plus a cross-check against a real value already recorded in a live `MergeInventory.xml`. Follow that pattern rather than assuming API docs alone are sufficient, particularly for anything hash- or serialization-related (see "Compatibility constraints" below). +- A few tests optionally cross-check against a real Witcher 3 + WitcherScriptMerger install (a live `MergeInventory.xml`'s recorded hashes, or a real `KDiff3.exe` binary a developer happens to have locally, for an auto-solvable-only A/B check against `DiffPlexMergeEngine` — WSM no longer bundles or requires one itself, see `docs/decisions/kdiff3-retirement.md`) via `WitcherScriptMerger.Tests/LiveInstall.cs`, gated entirely on the `WSM_TEST_GAME_DIR` environment variable (unset by default) — never a hardcoded or scanned path, per this repo's "scrub machine-specific paths" rule (see `CONTRIBUTING.md`). They silently no-op when it's unset. +- For anything not covered by the test project — especially further hash-output or `MergeInventory.xml` schema changes — the precedent set in `HANDOFF.md` still applies: a disposable, non-committed `dotnet new console` scratch app, exercising synthetic edge cases plus a cross-check against a real value already recorded in a live `MergeInventory.xml`. Follow that pattern rather than assuming API docs alone are sufficient, particularly for anything hash- or serialization-related (see "Compatibility constraints" below). ## Architecture Three projects as of the Core/host split plus the later addition of a test project (see git history around "Split into WitcherScriptMerger.Core + host GUI/CLI/MCP project" for the Core/host rationale): - **`WitcherScriptMerger.Core`** (`WitcherScriptMerger.Core/WitcherScriptMerger.Core.csproj`, `net10.0`, no WinForms reference — deliberately cross-platform-capable, toward eventual Linux support) holds all domain logic: file scanning, merge orchestration, load-order handling, settings/paths, and the CLI/MCP entry-point logic. Nothing in Core references `System.Windows.Forms`. -- **`WitcherScriptMerger`** (`WitcherScriptMerger/WitcherScriptMerger.csproj`, `net10.0-windows7.0`, `WinExe`, `UseWindowsForms=true`) is the host: WinForms GUI (`Forms/`, `Controls/`), the three entry points (`Program.cs`), and the one remaining external-tool wrapper with Win32 P/Invoke (`Tools/KDiff3.cs`). References Core via `ProjectReference`. +- **`WitcherScriptMerger`** (`WitcherScriptMerger/WitcherScriptMerger.csproj`, `net10.0-windows7.0`, `WinExe`, `UseWindowsForms=true`) is the host: WinForms GUI (`Forms/`, `Controls/`) and the three entry points (`Program.cs`). References Core via `ProjectReference`. No longer has a `Tools/` folder of its own — its one Win32-P/Invoke-using file, `Tools/KDiff3.cs`, was deleted along with KDiff3 (see `docs/decisions/kdiff3-retirement.md`). - **`WitcherScriptMerger.Tests`** (`WitcherScriptMerger.Tests/WitcherScriptMerger.Tests.csproj`, xunit, `net10.0`) references Core only — see "Tests" above for what it covers and its `AppState.Settings`-safety constraints. There is no MVC/MVP split within the host — `Forms/MainForm.cs` (~1000 lines) is a monolithic orchestrator that directly owns the tree controls, constructs `ModFileIndex`/drives merges, and wires up async callbacks. @@ -39,42 +39,48 @@ Folder map — **Core**: - `FileIndex/` — scans the mods folder and builds the conflict index: `ModFileIndex.cs` (`BuildAsync` → `Conflicts`), `ModFile.cs`, `ModFileCategory.cs`. - `Inventory/` — core merge domain + persistence: `FileMerger.cs` (headless orchestration plus TreeNode-free interactive orchestration — see "Interactive vs. headless split" below), `Merge.cs`, `MergeInventory.cs`, `FileHash.cs`, `MergeProgressInfo.cs`. - `LoadOrder/` — mod load-order logic: `CustomLoadOrder.cs`, `LoadOrderComparer.cs`, `LoadOrderValidator.cs`, `ModLoadSetting.cs`. -- `Tools/` — wrappers that shell out to bundled external executables, plus the merge-engine abstraction: `QuickBms.cs`, `WccLite.cs`, `Hasher.cs`, `IMergeEngine.cs` (see "Interactive vs. headless split" below), `FileEncoding.cs` (UTF-16LE+BOM normalization shared by every merge engine — see "KDiff3 input encoding" under Compatibility constraints), `DiffPlexMergeEngine.cs` (the DiffPlex-based `IMergeEngine` implementation — see "Interactive vs. headless split" below). +- `Tools/` — wrappers that shell out to bundled external executables, plus the sole text-merge engine: `QuickBms.cs`, `WccLite.cs`, `Hasher.cs`, `FileEncoding.cs` (UTF-16LE+BOM normalization used by `DiffPlexMergeEngine` — see "Text-merge input encoding" under Compatibility constraints), `DiffPlexMergeEngine.cs` (the sole text-merge engine — see "Interactive vs. headless split" below; KDiff3 and the `IMergeEngine` interface that used to sit in front of it were retired, see `docs/decisions/kdiff3-retirement.md`), `FileOpener.cs` (portable "open in the OS's default associated app" helper — the only call site is `DiffPlexMergeEngine.MergeHeadless`, opening a genuine conflict's marker sidecar). - `Cli/` — `MergeOperations.cs`: the scan-then-merge sequence shared by the `merge` CLI verb and the MCP tools. - `Mcp/` — `WsmMcpTools.cs`: the MCP server's tool implementations. See "MCP mode" below. -- Root: `AppState.cs` (shared mutable state — `Notifier`/`Settings`/`LoadOrder`/`Inventory`/`MergeEngine`), `AppSettings.cs`, `Paths.cs`, `StringExtensions.cs`, `IMergeNotifier.cs`, `NotifyTypes.cs` (the neutral `NotifyResult`/`NotifyButtons`/`DialogIcon` enums), `HeadlessMergeNotifier.cs`. +- Root: `AppState.cs` (shared mutable state — `Notifier`/`Settings`/`LoadOrder`/`Inventory`), `AppSettings.cs`, `Paths.cs`, `StringExtensions.cs`, `IMergeNotifier.cs`, `NotifyTypes.cs` (the neutral `NotifyResult`/`NotifyButtons`/`DialogIcon` enums), `HeadlessMergeNotifier.cs`. Folder map — **host**: - `Forms/` — WinForms screens: `MainForm.cs` (the hub, also implements `IMergeNotifier`, translating to/from real WinForms types), `OptionsForm.cs`, `DependencyForm.cs` (startup blocker if tool paths are invalid), `MergeReportForm.cs`, `PackReportForm.cs`, `PriorityPrompt.cs`, `MessageBoxManager.cs`. - `Controls/` — custom `TreeView` subclasses: `SMTree.cs` (base, metadata/context-menu logic), `ConflictTree.cs` (detected conflicts), `MergeTree.cs` (existing merges), `SMTreeSorter.cs`, `ToolStripRegion.cs`. - `Inventory/` — `InteractiveMergeRunner.cs`: the host-side counterpart to Core's `FileMerger` for the interactive path (see "Interactive vs. headless split" below). -- `Tools/` — `KDiff3.cs` (Win32 P/Invoke for window-title polling — stays host-only for now; a later unit removes it entirely), `KDiff3MergeEngine.cs` (the one real `IMergeEngine` implementation using KDiff3 — `DiffPlexMergeEngine`, Core, is the other one; see "Interactive vs. headless split" below for both). +- `Tools/` — empty as of KDiff3's retirement (`docs/decisions/kdiff3-retirement.md`); used to hold `KDiff3.cs` (Win32 P/Invoke for window-title polling) and `KDiff3MergeEngine.cs`. The sole text-merge engine, `DiffPlexMergeEngine`, lives in Core — see "Interactive vs. headless split" below. - Root: `Program.cs` (entry point: GUI, CLI, and MCP), `Extensions.cs` (WinForms-specific `TreeNode`/`TreeView` helpers and Win32 P/Invoke — pure string helpers live in Core's `StringExtensions.cs` instead), `TaskbarProgress.cs`, `App.config`. -### Interactive vs. headless split (`FileMerger` / `IMergeEngine`) +### Interactive vs. headless split (`FileMerger` / `DiffPlexMergeEngine`) Core's `FileMerger` never sees a `TreeNode`, `BackgroundWorker`, or `Forms.*` type. Its headless methods (`MergeConflictsHeadless` et al.) are unchanged in shape from before the split. Its interactive methods (`MergeFilesInteractive`, `MergeFlatFileInteractive`, `MergeBundleFileInteractive`, `MergeTextInteractive`) take a plain `InteractiveMergeRequest` (relative path, bundle flag, vanilla file path, ordered `MergeSource[]`) instead of `TreeNode[]`, and report back through `OnMergeReport`/`OnPackReport` callbacks instead of constructing `MergeReportForm`/`PackReportForm` directly. The host's `Inventory/InteractiveMergeRunner.cs` is the thing `MainForm` actually talks to: it extracts `InteractiveMergeRequest`s from checked `TreeNode`s (inside its `BackgroundWorker`'s `DoWork`, matching the pre-split threading model — extracting outside `DoWork` let a bad node throw synchronously on the UI thread instead of being captured by `BackgroundWorker`), owns the `BackgroundWorker`, and supplies the `OnMergeReport`/`OnPackReport` callbacks (report forms, completion sounds). -Both `FileMerger.MergeText*` methods talk to the active text-merge engine through `IMergeEngine` (`Merge`/`MergeHeadless`) rather than calling a specific tool directly. Two implementations exist: -- **`KDiff3MergeEngine`** (host) wraps `Tools/KDiff3.Run`/`KDiff3.RunHeadless` (Win32 P/Invoke has to stay in the host project) — see the "KDiff3 input encoding"/"Verify KDiff3 process behavior"/"KDiff3's pop-up window"/`RunHeadless`'s poll interval bullets under Compatibility constraints for what it's actually doing. -- **`DiffPlexMergeEngine`** (Core, `Tools/DiffPlexMergeEngine.cs`) is an in-process alternative built on the DiffPlex NuGet package (MIT-licensed), needing no external binary. It builds its own merge loop around `DiffPlex.ThreeWayDiffer.CreateDiffs` rather than calling `ThreeWayDiffer.CreateMerge` directly, so it can intercept `ThreeWayChangeType.Conflict` blocks itself: a conflict whose two sides are equal once whitespace is collapsed (joined-and-collapsed comparison over the classic ASCII whitespace set — space/tab/CR/LF/form-feed/vertical-tab, deliberately narrower than .NET regex's Unicode-aware `\s` so a genuine content difference that happens to be NBSP-vs-space isn't misclassified as whitespace-only — not per-line `Trim()`, and never applied when either side has zero pieces, since a real deletion must never be conflated with "the surviving side happens to collapse to empty too") auto-resolves by taking the first mod's side verbatim, mirroring KDiff3's `--cs "WhiteSpace3FileMergeDefault=2"` (confirmed against the KDiff3 source: value 2 means "always pick input B", and KDiff3's own file order — vanilla, source1, source2 — maps source1 to B). A genuine conflict instead produces git/diff3-style conflict markers (`<<<<<<< ` / `||||||| Vanilla` / `=======` / `>>>>>>> `) written to a **sidecar** file under `Paths.DiffPlexConflictsDirectory` (a dedicated top-level `DiffPlexConflicts` folder — via `GetConflictMarkerPath`, keyed by an `XxHash32` of the full output path plus its filename) rather than to `outputPath` itself — writing markers to `outputPath` would make `FileMerger`'s pre-merge `File.Exists(_outputPath)` overwrite guard treat it as an already-completed merge and permanently skip retrying, since `HeadlessMergeNotifier` always declines the overwrite prompt. The sidecar went through two prior locations before landing here, each ruled out by direct end-to-end testing against the real CLI rather than by inspection alone: it originally lived right beside `outputPath` (`.conflict`), which code review flagged for two real problems (a flat-file conflict's `outputPath` sits inside the live `Paths.ModsDirectory` tree, which nothing ever cleans up; a bundle-content conflict's `outputPath` sits inside `Paths.MergedBundleContent`, which `Tools/WccLite.PackBundle` packs *wholesale* with no filtering, so a leftover `.conflict` file there could get embedded as bogus content into a later-packed `blob0.bundle`); it then moved under `Paths.TempBundleContent`, which fixed both of those but broke immediately in end-to-end testing for a third reason neither review nor unit tests caught - `FileMerger.CleanUpTempFiles()` deletes the entire `TempBundleContent` tree wholesale at the end of every headless merge run (to clear QuickBMS-unpacked bundle scratch content), so the sidecar was gone by the time the CLI process exited, before a user could ever see it. `Paths.DiffPlexConflictsDirectory` is an unrelated top-level name specifically to avoid that collision - see its own comment in `Paths.cs` for the full story. A conflict-marker file that later becomes an auto-solve on retry has its stale sidecar deleted before writing the fresh output. Two loose ends worth stating plainly rather than leaving implicit: nothing automatically deletes the `DiffPlexConflicts` directory itself (the same "accumulates until manually cleared" property `TempBundleContent` has, and the CLI-mode section below already tells users to clear that one between runs - `DiffPlexConflicts` needs the same manual housekeeping, just without an automated sweep); and `Paths.DiffPlexConflictsDirectory` is a relative path, resolved against `Environment.CurrentDirectory` - `Program.RunCli` sets that to `AppContext.BaseDirectory` before anything else runs (see "Startup flow" below), so in CLI mode sidecars land predictably next to the installed exe, but the GUI path never does that reset, so a GUI-mode DiffPlex conflict's sidecar lands wherever the process's CWD happened to be at launch (typically the exe's own directory too, for a normal double-click launch, but not guaranteed). Not a functional problem given the GUI-mode DiffPlex path is already incomplete UI-wise (no way to open the sidecar from the report dialog yet either), but worth knowing before relying on the sidecar's location being deterministic outside CLI mode. There's no UI here at all (unlike KDiff3's own GUI), so `Merge()` (interactive) just runs `MergeHeadless()` and maps `NeedsManualResolution` to `Failed`, matching `IMergeEngine.Merge`'s contract — this also means the "merging an updated mod file into an existing merge chain" outdated-hash guard (mirrored from `KDiff3.RunHeadless`, not hoisted into shared `FileMerger` orchestration) surfaces as a silent `Failed` on the interactive path here, where `KDiff3MergeEngine`'s interactive `Run()` opens KDiff3's GUI for manual review instead — an accepted gap given there's no interactive UI for DiffPlex conflicts at all yet. A 3-way merge with no vanilla version at all (expected mainly on the bundle-content path, when no matching vanilla bundle is found, but the guard applies unconditionally to any conflict missing one) is refused outright (`NeedsManualResolution`, nothing written) rather than attempted with an empty base string — `DiffPlex.ThreeWayDiffer` degrades silently to zero diff blocks and a "successful" empty merge in that case, confirmed empirically, which would otherwise produce a truncated output file; this is a deliberate divergence from `KDiff3MergeEngine`, which has no equivalent guard and always attempts a real (if vanilla-less, degraded 2-way) `--auto` merge instead, since KDiff3 has a coherent notion of a 2-file merge and DiffPlex's `ThreeWayDiffer` as used here does not. See the "DiffPlex's `ThreeWayDiffer` can produce internally inconsistent diff blocks" bullet under Compatibility constraints below for a confirmed upstream DiffPlex bug this engine has to defend against on every merge, regardless of any of the above. Selected via the `MergeEngine` `App.config` key (`kdiff3`, the default, or `diffplex`) — see "Startup flow" below; `KDiff3MergeEngine` remains the default, both because `DiffPlexMergeEngine` hasn't been cross-checked against KDiff3 on enough real conflicting files yet (only synthetic fixtures in `WitcherScriptMerger.Tests`, plus an optional, narrowly-scoped real-KDiff3 A/B check gated on `WSM_TEST_GAME_DIR` — see "Tests" above) and because of that confirmed upstream DiffPlex bug's measured non-trivial failure rate even at realistic edit densities. +Both `FileMerger.MergeText*` methods talk to a private `DiffPlexMergeEngine` field (`Merge`/`MergeHeadless`) constructed by `FileMerger`'s own constructor. There used to be an `IMergeEngine` interface between them, with two implementations — `KDiff3MergeEngine` (host, wrapping `Tools/KDiff3.Run`/`KDiff3.RunHeadless`) and `DiffPlexMergeEngine` (Core) — selectable via a `MergeEngine` App.config key. KDiff3 was retired (see `docs/decisions/kdiff3-retirement.md` for the full rationale and the empirical KDiff3 process-behavior findings preserved there); with only one implementation ever going to remain, the interface indirection was deleted along with it, per this repo's own "no premature abstraction" convention (see CONTRIBUTING.md) — `FileMerger` now calls `DiffPlexMergeEngine` directly. -Either way, `AppState.MergeEngine` is supplied once, as (part of) the first line of `Program.Main`, before anything else runs. `IMergeEngine` is explicitly scaffolding for the Core/host split, not a permanent pluggable-engine abstraction — a later unit removing KDiff3 entirely will likely delete this interface and inline its replacement directly into `FileMerger`. +**`DiffPlexMergeEngine`** (Core, `Tools/DiffPlexMergeEngine.cs`) is the sole text-merge engine — in-process, built on the DiffPlex NuGet package (MIT-licensed), needing no external binary. It builds its own merge loop around `DiffPlex.ThreeWayDiffer.CreateDiffs` rather than calling `ThreeWayDiffer.CreateMerge` directly, so it can intercept `ThreeWayChangeType.Conflict` blocks itself: a conflict whose two sides are equal once whitespace is collapsed (joined-and-collapsed comparison over the classic ASCII whitespace set — space/tab/CR/LF/form-feed/vertical-tab, deliberately narrower than .NET regex's Unicode-aware `\s` so a genuine content difference that happens to be NBSP-vs-space isn't misclassified as whitespace-only — not per-line `Trim()`, and never applied when either side has zero pieces, since a real deletion must never be conflated with "the surviving side happens to collapse to empty too") auto-resolves by taking the first mod's side verbatim, mirroring the now-retired KDiff3 engine's `--cs "WhiteSpace3FileMergeDefault=2"` (confirmed against the KDiff3 source, preserved in `docs/decisions/kdiff3-retirement.md`: value 2 means "always pick input B", and KDiff3's own file order — vanilla, source1, source2 — maps source1 to B). A genuine conflict instead produces git/diff3-style conflict markers (`<<<<<<< ` / `||||||| Vanilla` / `=======` / `>>>>>>> `) written to a **sidecar** file under `Paths.DiffPlexConflictsDirectory` (a dedicated top-level `DiffPlexConflicts` folder — via `GetConflictMarkerPath`, keyed by an `XxHash32` of the full output path plus its filename) rather than to `outputPath` itself — writing markers to `outputPath` would make `FileMerger`'s pre-merge `File.Exists(_outputPath)` overwrite guard treat it as an already-completed merge and permanently skip retrying, since `HeadlessMergeNotifier` always declines the overwrite prompt. The sidecar went through two prior locations before landing here, each ruled out by direct end-to-end testing against the real CLI rather than by inspection alone: it originally lived right beside `outputPath` (`.conflict`), which code review flagged for two real problems (a flat-file conflict's `outputPath` sits inside the live `Paths.ModsDirectory` tree, which nothing ever cleans up; a bundle-content conflict's `outputPath` sits inside `Paths.MergedBundleContent`, which `Tools/WccLite.PackBundle` packs *wholesale* with no filtering, so a leftover `.conflict` file there could get embedded as bogus content into a later-packed `blob0.bundle`); it then moved under `Paths.TempBundleContent`, which fixed both of those but broke immediately in end-to-end testing for a third reason neither review nor unit tests caught - `FileMerger.CleanUpTempFiles()` deletes the entire `TempBundleContent` tree wholesale at the end of every headless merge run (to clear QuickBMS-unpacked bundle scratch content), so the sidecar was gone by the time the CLI process exited, before a user could ever see it. `Paths.DiffPlexConflictsDirectory` is an unrelated top-level name specifically to avoid that collision - see its own comment in `Paths.cs` for the full story. A conflict-marker file that later becomes an auto-solve on retry has its stale sidecar deleted before writing the fresh output. + +**The sidecar is opened for the user, not just written.** Immediately after writing it and reporting the skip via `AppState.Notifier.ShowMessage`, `MergeHeadless` calls `Tools/FileOpener.Open` (a swappable static `Func`, defaulting to `Process.Start` with `UseShellExecute = true` so it resolves the OS's file association rather than trying to execute the sidecar as a process image) on the sidecar path — opening it in the user's default editor for that file type. Since there's no UI here at all, `Merge()` (interactive) just runs `MergeHeadless()` and maps `NeedsManualResolution` to `Failed` — this also means the sidecar-write-and-open behavior, and the "merging an updated mod file into an existing merge chain" outdated-hash guard, both fire identically whether reached via the GUI's interactive path or the CLI/MCP headless path, since it's the exact same underlying call either way. Deliberately not `Program.TryOpenFile` (the host's existing, WinForms-adjacent equivalent, used elsewhere for opening merged output files): that helper's non-`.exe` branch is a bare `Process.Start(path)` with no `UseShellExecute = true`, which on modern .NET (unlike .NET Framework, where `UseShellExecute` defaulted to true) throws for a non-executable path — silently swallowed by that method's surrounding `catch`, and out of scope to fix there, but not a pattern worth propagating into this new code path. `Tools/FileOpener.cs` exists specifically so both the GUI-interactive and CLI/MCP-headless paths can reach a correctly-implemented version of this without Core referencing `System.Windows.Forms`. + +Two loose ends worth stating plainly rather than leaving implicit: nothing automatically deletes the `DiffPlexConflicts` directory itself (the same "accumulates until manually cleared" property `TempBundleContent` has, and the CLI-mode section below already tells users to clear that one between runs - `DiffPlexConflicts` needs the same manual housekeeping, just without an automated sweep); and `Paths.DiffPlexConflictsDirectory` is a relative path, resolved against `Environment.CurrentDirectory` - `Program.RunCli` sets that to `AppContext.BaseDirectory` before anything else runs (see "Startup flow" below), so in CLI mode sidecars land predictably next to the installed exe, but the GUI path never does that reset, so a GUI-mode conflict's sidecar lands wherever the process's CWD happened to be at launch (typically the exe's own directory too, for a normal double-click launch, but not guaranteed). + +A 3-way merge with no vanilla version at all (expected mainly on the bundle-content path, when no matching vanilla bundle is found, but the guard applies unconditionally to any conflict missing one) is refused outright (`NeedsManualResolution`, nothing written) rather than attempted with an empty base string — `DiffPlex.ThreeWayDiffer` degrades silently to zero diff blocks and a "successful" empty merge in that case, confirmed empirically, which would otherwise produce a truncated output file; this is a deliberate divergence from the now-retired KDiff3 engine, which had no equivalent guard and always attempted a real (if vanilla-less, degraded 2-way) `--auto` merge instead, since KDiff3 had a coherent notion of a 2-file merge that DiffPlex's `ThreeWayDiffer`, as used here, does not (see `docs/decisions/kdiff3-retirement.md`). See the "DiffPlex's `ThreeWayDiffer` can produce internally inconsistent diff blocks" bullet under Compatibility constraints below for a confirmed upstream DiffPlex bug this engine has to defend against on every merge, regardless of any of the above — the primary reason this retirement accepted a real, measured reliability gap in exchange for dropping KDiff3, rather than treating DiffPlex as a strict improvement. + +`FileMerger`'s constructor (`public FileMerger(MergeInventory inventory)`) builds its own `DiffPlexMergeEngine` — there's no longer an engine selection step at startup. ### Startup flow (`Program.cs`) The shared mutable state that used to be static fields directly on `Program` (`Notifier`/`Settings`/`LoadOrder`/`Inventory`) now lives on Core's `AppState` instead — domain code that moved to Core needs to read/write it, and Core can never reference the host assembly. `Program.Notifier`/`Settings`/`LoadOrder`/`Inventory` are pass-through properties onto `AppState` so every pre-existing host call site kept working unchanged. `AppState.Notifier` defaults to `HeadlessMergeNotifier` via field initializer, unconditionally, so any startup error is safe to report even before it's known whether this is a GUI or CLI run. `AppState.Settings` is a **lazy property**, not a field initializer (`_settings ?? (_settings = new AppSettings())`) — deliberately decoupled from `Notifier`'s eager init: `AppSettings`'s constructor calls `Environment.Exit(1)` if it can't find a config file next to the entry assembly, appropriate for the real GUI/CLI/MCP entry points (where that's genuinely fatal) but not for `WitcherScriptMerger.Tests`, whose `dotnet test` host has no matching `.config` — Core code legitimately reads `AppState.Notifier` on its own (e.g. `DiffPlexMergeEngine`'s headless skip/guard messages), and before this change that alone was enough to also force `Settings` to construct (C# runs all of a type's static field initializers together on first touch of *any* static member) and crash the whole test process. First real access to `Settings` still runs the identical `new AppSettings()` and identical crash-on-missing-config behavior for the real app, just deferred to that access instead of bundled with `Notifier`'s. `AppState` has an explicit (empty) static constructor so its own init ordering is deterministic rather than left to `beforefieldinit`'s discretion — `Program` needs the same treatment for its own remaining field initializer (`_consoleAttached = MaybeAttachConsole()`) for the identical reason, now that nothing in `Main()` necessarily touches a `Program`-owned field anymore (its former fields became properties). `MaybeAttachConsole()` runs as a field initializer, ahead of everything, so early failures are visible in the invoking terminal when there are CLI args. -`[STAThread] Main(string[] args)`: first sets `AppState.MergeEngine` to either `KDiff3MergeEngine` (default) or `DiffPlexMergeEngine`, based on the `MergeEngine` `App.config` setting (`kdiff3`/`diffplex`) — see "Interactive vs. headless split" above; must happen before anything calls `Paths.ValidateDependencyPaths()` or constructs a `FileMerger`, in any of the paths below. Then: if `args` is non-empty, hands off entirely to the CLI path (see below) and returns — the GUI is never touched. Otherwise: `Application.EnableVisualStyles()` → check `Settings.HasConfigFile` → `Paths.ValidateDependencyPaths()` (shows `DependencyForm` if KDiff3/QuickBMS/wcc_lite paths are invalid) → construct `MainForm`, reassign `Program.Notifier = MainForm`, `Application.Run(MainForm)`. +`[STAThread] Main(string[] args)`: if `args` is non-empty, hands off entirely to the CLI path (see below) and returns — the GUI is never touched. Otherwise: `Application.EnableVisualStyles()` → check `Settings.HasConfigFile` → `Paths.ValidateDependencyPaths()` (shows `DependencyForm` if QuickBMS/wcc_lite paths are invalid) → construct `MainForm`, reassign `Program.Notifier = MainForm`, `Application.Run(MainForm)`. (There used to be an engine-selection step here, setting `AppState.MergeEngine` to `KDiff3MergeEngine` or `DiffPlexMergeEngine` based on a `MergeEngine` App.config setting, before anything else ran — removed along with `IMergeEngine` itself; see "Interactive vs. headless split" above and `docs/decisions/kdiff3-retirement.md`.) ### Merge flow -No hand-rolled diff algorithm lives in this codebase — it's an orchestrator around KDiff3 for text merges and QuickBMS/wcc_lite for `.bundle` archives: +No hand-rolled diff algorithm lives in this codebase — it's an orchestrator around an in-process DiffPlex-based engine for text merges (KDiff3 formerly filled this role; see `docs/decisions/kdiff3-retirement.md`) and QuickBMS/wcc_lite for `.bundle` archives: 1. `FileIndex/ModFileIndex.BuildAsync` scans `Paths.ModsDirectory`, groups files by relative path, flags conflicts. 2. `MainForm` feeds the results into `ConflictTree`/`MergeTree`; the user checks nodes to merge. -3. `Inventory/FileMerger.MergeByTreeNodesAsync` runs on a `BackgroundWorker`, building/reusing an `Inventory/Merge` record per file and dispatching to `MergeFlatFileNode` (plain `.ws`/`.xml`) or `MergeBundleFileNode` (bundle-packed files, which first go through `Tools/QuickBms.UnpackFile`). -4. `FileMerger.MergeText` calls `Tools/KDiff3.Run(source1, source2, vanillaFile, outputPath)`, which shells out to `KDiff3.exe` (`--auto` for auto-solvable 3-way merges, or opens its GUI for manual resolution). Before building the command line, `KDiff3.Run` normalizes each input file to UTF-16LE with a BOM (matching vanilla's encoding) via `Tools/FileEncoding.EnsureUtf16File` (Core - shared with `DiffPlexMergeEngine`, see "Interactive vs. headless split" above), writing a temp copy under `Paths.TempBundleContent\Encoding\...` when a file isn't already in that encoding — see "KDiff3 input encoding" below for why. +3. The host's `Inventory/InteractiveMergeRunner` extracts an `InteractiveMergeRequest` per checked `TreeNode` and runs `FileMerger.MergeFilesInteractive` on a `BackgroundWorker`; `MergeFilesInteractive` builds/reuses an `Inventory/Merge` record per file and dispatches to `MergeFlatFileInteractive` (plain `.ws`/`.xml`) or `MergeBundleFileInteractive` (bundle-packed files, which first go through `Tools/QuickBms.UnpackFile`) — see "Interactive vs. headless split" above for the full Core/host breakdown. +4. `FileMerger.MergeTextInteractive` calls `DiffPlexMergeEngine.Merge(source1, source2, vanillaFile, outputPath)`, which merges in-process (`--auto`-equivalent auto-solving; a genuine conflict writes and opens a conflict-marker sidecar instead of opening a merge GUI - see "Interactive vs. headless split" above). Before merging, it normalizes each input's text to UTF-16LE (matching vanilla's encoding) via `Tools/FileEncoding.ReadAnyEncoding` - see "Text-merge input encoding" below for why. 5. On success, `Inventory/MergeInventory.AddModToMerge` hashes the result via `Tools/Hasher` (xxHash32, `System.IO.Hashing`) and persists the merge record to `MergeInventory.xml` (`XmlSerializer`). `MergeInventory.HasResolvedConflict` re-checks these hashes on refresh to detect merges made stale by upstream mod file changes. 6. Bundle content changes additionally go through `FileMerger.PackNewBundle` → `Tools/WccLite.PackBundle` + `GenerateMetadata` to repack `blob0.bundle`. @@ -82,11 +88,11 @@ The CLI path (`FileMerger.MergeConflictsHeadless`) is a separate, parallel orche ### CLI mode -`WitcherScriptMerger.exe merge [--order-file ]` merges every auto-solvable conflict without opening any window, then exits. No-args still launches the GUI unchanged; passing `merge` is what selects the CLI path in `Program.cs`. +`WitcherScriptMerger.exe merge [--order-file ]` merges every auto-solvable conflict without opening any merge-tool window, then exits (a conflict needing manual resolution does open its conflict-marker sidecar in the default editor - see "Interactive vs. headless split" above - but that's a text file opening in whatever app is associated with it, not a dedicated merge-tool UI). No-args still launches the GUI unchanged; passing `merge` is what selects the CLI path in `Program.cs`. - **Orchestration**: `Cli/MergeOperations.ScanConflicts()` runs `ModFileIndex.BuildAsync` synchronously (via a `ManualResetEventSlim`) and returns the built index; `MergeOperations.RunMerge(inventory, conflicts, mergedModName, orderOverrides)` calls `FileMerger.MergeConflictsHeadless`. Both the `merge` CLI verb and the MCP tools (see "MCP mode" below) call these instead of duplicating the scan/wait/merge sequence. `MergeConflictsHeadless` iterates `ModFileIndex.Conflicts` directly — those are plain `ModFile`/`FileHash` objects (relative path, category, per-mod name and hash), so no `TreeNode`/`ConflictTree` is ever constructed for this path. `MergeFlatConflictHeadless` and `MergeBundleConflictHeadless` mirror `MergeFlatFileNode`/`MergeBundleFileNode`'s logic against that plain data instead of `TreeNode.GetMetadata()`; bundle merges reuse `GetUnpackedFiles`/`PackNewBundle` unchanged. Per-file mod order defaults to `LoadOrderComparer` (matching `ConflictTree`'s own default sort, `Controls/SMTreeSorter.cs`); the `--order-file` JSON (`{"relative\\path.ws": ["modA", "modB"]}`) overrides specific files without requiring every conflict to be listed. Within a listed file's mod list, `FileMerger.ResolveMergeOrder` requires: no unknown mod names, no duplicate names, at least two entries, and every one of that file's *real* source mods present at least once — any violation rejects that one file with a clear error (via `AppState.Notifier.ShowError`) rather than silently merging an incomplete, self-paired, or single-entry (no-op) chain. This applies to both the CLI's `--order-file` and the MCP `merge_conflicts` tool's `orderOverrides`, since both funnel through this shared method. "Real source mods" deliberately excludes the configured merged-mod name itself: once a file has already been merged once, its own merged-mod folder re-enters `conflict.Mods` as if it were a source (see `scan_conflicts`'s tool description below), so re-merging after a source mod's file changes only needs to list the actual mods again, not the previous merge output too — the at-least-two-entries rule still applies, though, so listing only that one remaining real mod (with the merged-mod folder excluded from the requirement) is rejected rather than silently merged with nothing to merge against. - **`IMergeNotifier`** (Core: `IMergeNotifier.cs`, `NotifyTypes.cs`, `HeadlessMergeNotifier.cs`; host: `MainForm.cs`'s implementation): replaces every direct `Program.MainForm.ShowMessage/ShowError/ShowModal` call in domain code with `AppState.Notifier.*` (host code still spells this `Program.Notifier.*`, via the pass-through property). The interface is defined against neutral `NotifyResult`/`NotifyButtons`/`DialogIcon` types, not `DialogResult`/`MessageBoxButtons`/`MessageBoxIcon` — Core can't reference `System.Windows.Forms` at all. `MainForm` translates those neutral types to/from real `MessageBox.Show(...)`/`DialogResult` calls; this is **not** a behavior-identical passthrough — one real prompt (`LoadOrderValidator`'s "Custom Load Order Problem" dialog) lost a `MessageBoxManager`-based custom button caption that used to mark its Cancel option as destructive/permanent (that caption mechanism was likely already silently broken pre-split — `MessageBoxManager.Register()`'s `AppDomain.GetCurrentThreadId()`-based hook doesn't reliably work on modern .NET — but the loss is real either way; the warning is now spelled out in the message body instead). `ShowModal(Form)` isn't part of `IMergeNotifier` at all — every call site is GUI-only, interactive code in the host project, which calls `MainForm.ShowModal` directly instead of going through the notifier abstraction. `HeadlessMergeNotifier` writes to the console and returns a fixed, non-destructive default for every decision: don't overwrite an existing merge output, don't use a merge name that's still conflicting, don't continue past a canceled/failed merge — except where a caller explicitly overrides that generic default via `ShowMessage`'s `defaultResult` parameter (added for `LoadOrderValidator`, whose YesNoCancel prompt has an inverted-from-usual safety shape: Cancel, not Yes/No, is the one destructive/permanent choice there). This is also what fixed a real null-ref hazard in `CustomLoadOrder.Refresh()`, which used to reach `Program.MainForm` at construction time. -- **`KDiff3.RunHeadless`**: see the "Verify KDiff3 process behavior..." and "KDiff3's pop-up window can't be suppressed" compatibility constraints above for the window-persistence detection this relies on and why the window itself is left alone rather than hidden. `-o` always targets a scratch path under `Paths.TempBundleContent\HeadlessOutput\`, copied to the real output only after a confirmed clean exit — a killed process can never leave a partial file at the real path. +- **`DiffPlexMergeEngine.MergeHeadless`**: see "Interactive vs. headless split" above for the full mechanics (whitespace-only auto-resolve, conflict-marker sidecar, `FileOpener.Open`). This used to be `KDiff3.RunHeadless`, which needed a window-persistence-detection poll loop, a scratch-output-then-copy-on-clean-exit pattern to guarantee a killed process could never leave a partial file at the real output path, and a best-effort foreground-focus restore after KDiff3's unsuppressible popup closed — none of that machinery is needed anymore, since this engine never opens a window at all. See `docs/decisions/kdiff3-retirement.md` for that machinery's full empirical writeup, preserved now that the code itself is gone. - **Verification status**: the flat-file path (`Categories.Script`/`Categories.Xml`) is verified end-to-end against real conflicting files (including the encoding-mismatch scenario) and a synthetic guaranteed-conflict, in a scratch game/mods tree — never against a live install. The bundle path (`Categories.BundleText`) is code-reviewed and mirrors the proven flat-file orchestration, and its two building blocks (`GetUnpackedFiles`, `PackNewBundle`) are unchanged, already-exercised code — but it hasn't been round-tripped through a real bundle-vs-bundle conflict. If `tempbundlecontent` accumulates across many CLI runs during development, clear it between runs — one debugging session saw a single very slow run after a long buildup that didn't reproduce once it was cleared. ### MCP mode @@ -97,17 +103,15 @@ The CLI path (`FileMerger.MergeConflictsHeadless`) is a separate, parallel orche - **Tools** (`Mcp/WsmMcpTools.cs`, `[McpServerToolType]` static class): `scan_conflicts` (no args — scans and returns every conflict's relative path, category, per-mod hashes, default merge order, and whether it's already resolved), `merge_conflicts` (optional `relativePaths` to restrict to specific files, optional `orderOverrides` — same shape as the CLI's `--order-file`, minus the file, and validated per-file the same way (see "CLI mode" above — no duplicates, every real source mod covered), optional `dryRun` to preview which conflicts would auto-solve without writing merged output, repacking a bundle, or touching `MergeInventory.xml`; returns `{merged: [...], skipped: [...], unmatched: [...], dryRun}` — `unmatched` lists any requested `relativePaths` entry that's in-scope but no longer a detected conflict), `get_status` (dependency validation, resolved game/mods directories, configured merged-mod name, current conflict count), `list_merges` (enumerates `MergeInventory.xml`'s existing `Merge` records). All four reuse `Cli/MergeOperations` and the same `IMergeNotifier`/`HeadlessMergeNotifier` machinery as the `merge` CLI verb — merge decisions get the same safe non-destructive defaults either way. - **Directory allow-listing**: `merge_conflicts`'s `relativePaths` and `orderOverrides` keys are validated (`WsmMcpTools.EnsureInScope`/`IsWithinModsDirectory`) to resolve inside `Paths.ModsDirectory` before any scan or merge runs — an absolute path, UNC path, or `..\`-escaping entry is rejected with a clear error instead of silently matching nothing. Neither value is actually joined into a filesystem path anywhere in this codebase today (`relativePaths` is only ever compared for equality against already-scanned `ModFile.RelativePath` values; `orderOverrides` values reach `Path.Combine` in `FileMerger.GetModFile` but only after being validated against `ModFile.ContainsMod`, a whitelist of real scanned mod folder names) — this check is defense-in-depth, not a fix for a live traversal. This check applies mods-directory-relative semantics uniformly to every category; for `Categories.BundleText` specifically, `conflict.RelativePath` is actually a path *internal to a bundle archive* (from `QuickBms.GetBundleContentPaths`), not one rooted at `Paths.ModsDirectory` — an ordinary internal path (e.g. `engine\foo.ws`) still validates fine, but a bundle whose internal listing itself contained a rooted or `..`-bearing entry would make that specific conflict unreachable via `relativePaths` (rejected as out-of-scope even though it's a legitimate conflict). Unexercised: every scratch config used to verify this unit has `CheckBundleContents=false`, consistent with the bundle path's existing "code-reviewed but not round-tripped" verification status below. See `Mcp/CLAUDE.md` for the minimal-permissions summary this unit added. - **State per call, not cached across calls**: every tool call re-scans (`ModFileIndex.BuildAsync`) and re-loads `MergeInventory.Load(Paths.Inventory)` fresh rather than keeping a long-lived server-side cache, since the mods folder or `MergeInventory.xml` can change between calls (including from a concurrently-running GUI instance) and there's no test suite to catch a staleness bug. -- **Verification status**: smoke-tested end-to-end against the same scratch game/mods tree used for CLI mode verification — `initialize`, `tools/list`, and all four tools called via a hand-rolled stdio client, including a `merge_conflicts` call that exercised the real `KDiff3.RunHeadless` path (detected a genuine conflict, killed the stuck process, returned it in `skipped`). Never run against a live install. The directory-allow-listing and `dryRun` additions were verified against a real KDiff3 stand-in (dependency paths present but not a real KDiff3.exe, so merges reliably fail/skip) via the same stdio-client approach, plus an in-process harness against `WitcherScriptMerger.Core` directly with a fake `IMergeEngine` (real KDiff3/QuickBMS/wcc_lite weren't available in that verification environment) — see the PR that introduced them for exactly what each covered. +- **Verification status**: smoke-tested end-to-end against the same scratch game/mods tree used for CLI mode verification — `initialize`, `tools/list`, and all four tools called via a hand-rolled stdio client, including a `merge_conflicts` call that exercised the (since-retired) `KDiff3.RunHeadless` path at the time (detected a genuine conflict, killed the stuck process, returned it in `skipped`) — the underlying engine has changed since (see `docs/decisions/kdiff3-retirement.md`), but the MCP-level behavior this verified (a conflict comes back in `skipped`, not silently dropped) is unaffected. Never run against a live install. The directory-allow-listing and `dryRun` additions were verified against a real KDiff3 stand-in (dependency paths present but not a real KDiff3.exe, so merges reliably fail/skip) via the same stdio-client approach, plus an in-process harness against `WitcherScriptMerger.Core` directly with a fake `IMergeEngine` (both now historical - `IMergeEngine` no longer exists; real KDiff3/QuickBMS/wcc_lite weren't available in that verification environment) — see the PR that introduced them for exactly what each covered. ### Compatibility constraints - **Hash format is load-bearing.** `MergeInventory.xml` (including real, already-populated files on developer machines) stores per-file hashes compared by string equality to detect when a mod source file has changed since it was last merged. Any change to `Tools/Hasher.cs` must produce byte-for-byte identical output to the current implementation, or every existing recorded merge silently "goes stale." Verify with the synthetic-edge-cases + real-recorded-hash cross-check pattern described under Tests. - **TFM must keep the explicit `7.0` OS-version suffix.** The project uses `false` (to keep the hand-written `Properties/AssemblyInfo.cs`), which also suppresses the SDK's auto-generated `[assembly: SupportedOSPlatform("windows")]` attribute — that attribute is added manually in `AssemblyInfo.cs` instead. The TFM must stay `net10.0-windows7.0` (not bare `net10.0-windows`); dropping the `7.0` suffix reintroduces ~800 spurious `CA1416` platform-compatibility warnings. -- **KDiff3 input encoding must stay normalized to UTF-16LE, never down to UTF-8.** Vanilla `.ws` files are UTF-16LE with a BOM; mod authors' files are often plain UTF-8/ASCII with no BOM (confirmed against real files on a live install). KDiff3 has no command-line flag to specify per-input encoding, and a mismatch can make it treat an entire file as unmatchable, falling back to manual (GUI) conflict resolution instead of auto-solving — empirically confirmed real-world false-conflict case: `baseEffect.ws` failed to auto-solve with mismatched encodings and succeeded cleanly once normalized, with correct merged output. `Tools/FileEncoding.cs` (Core, shared by both merge engines) handles this: `EnsureUtf16File` writes a UTF-16LE+BOM temp copy of any non-UTF-16LE input file before invoking KDiff3 (matching vanilla's encoding), while `ReadAnyEncoding`/`WriteUtf16` give `DiffPlexMergeEngine` the same normalization without needing a temp file at all, since it merges in-process text rather than shelling out to a tool that needs a file path — never normalize toward UTF-8, since the game may not load a merged `.ws` file in that encoding. `ReadAnyEncoding` deliberately uses `File.ReadAllText(path)`'s built-in BOM auto-detection rather than decoding raw bytes with a fixed `Encoding` instance - the latter does not strip a detected BOM, leaving a stray U+FEFF glued to the first line, confirmed empirically to reproduce the exact `baseEffect.ws`-style false conflict this whole mechanism exists to avoid (see `WitcherScriptMerger.Tests`' `MergeHeadless_EncodingMismatch_...` fixture). KDiff3 itself has no config option to ignore whitespace generally during diff/merge (only `WhiteSpace3FileMergeDefault` auto-picks a side for *purely* whitespace-only conflicts, confirmed against the KDiff3 source: value `2` means "always pick input B", i.e. the first mod/`source1`, given KDiff3's own file order of vanilla/source1/source2) — confirmed from the bundled `doc/options.html`, not assumed. `DiffPlexMergeEngine` mirrors this for conflicts that are purely whitespace once collapsed (see "Interactive vs. headless split" above), rather than only literal-identical-content merges. -- **DiffPlex's `ThreeWayDiffer` (1.9.0) can produce internally inconsistent diff blocks - `DiffPlexMergeEngine.BuildMerge` must never trust its output without checking.** Confirmed as a genuine upstream library bug, not a defect in this repo's own merge loop: `BuildMerge`'s block-iteration loop is a faithful port of DiffPlex's own `ThreeWayDiffer.CreateMerge` (same index-chasing shape), and a throwaway scratch console app (per this repo's testing convention) calling DiffPlex's own `CreateMerge` directly - with both `LineChunker` (DiffPlex's own default, and the *only* chunker its own `Facts.DiffPlex/ThreeWayDifferFacts.cs` test suite ever exercises for 3-way diffs) and `LineEndingsPreservingChunker` (the one this engine actually uses) - reproduced the identical failure on the identical input either way. When old-side and new-side edits interleave/overlap relative to base in certain ways, `CreateThreeWayDiffBlocks` can emit a block list whose `OldCount`/`NewCount` don't actually correspond to the real `PiecesOld`/`PiecesNew` arrays. This surfaces two ways: an outright `ArgumentOutOfRangeException` from direct indexer access, or - confirmed via a minimal repro (base `"a();/b();/c();"`, one side inserts a line after `a()`, the other independently changes `b()` to `B()`) - no exception at all, but silently wrong output (content lost or duplicated), because the running `oldIndex`/`newIndex` end up not matching `PiecesOld.Count`/`PiecesNew.Count` even though no single block's own bookkeeping looked wrong in isolation. A large randomized stress test (varying edit density and file length, run against the real, fixed `BuildMerge`) measured combined failure rates of **0.35%** at one independent single-line edit per side on 50-200 line files (the closest analogue to a typical two-mod `.ws` conflict), rising to **0.88%** (1-2 edits/side), **2.65%** (2-3 edits/side), **4.99%** (1-6 edits/side on 50-200 line files), and **38.89%** on the original dense adversarial case (1-6 edits/side on 1-19 line files) - zero cases of any exception type other than the one this bug produces, across 100,000 total trials. `BuildMerge` defends against both failure modes: the block-processing loop is wrapped in `try`/`catch (ArgumentOutOfRangeException)`, and a post-loop check verifies `oldIndex`/`newIndex` actually reached `PiecesOld.Count`/`PiecesNew.Count` (accounting for a legitimate trailing-unchanged gap needing the exact same lockstep advance as the per-block gap-catchup above it - an early, incorrect version of this check that skipped that trailing advance produced a ~33% false-positive "inconsistent" rate on the exact same benign inputs). Either failure mode throws a `DiffPlexMergeEngine.DiffAlgorithmException`, which `MergeHeadless` catches and reports as `NeedsManualResolution` **without writing anything, including a conflict-marker sidecar** - the marker content itself would have been built from the same untrustworthy piece indices, so this is the one case where `DiffPlexMergeEngine` can't even offer a conflict-marker starting point the way KDiff3 always can. This is the primary reason `DiffPlexMergeEngine` isn't the default engine (see "Interactive vs. headless split" above) - a measured, non-negligible failure rate even at realistic edit density is a real reliability gap `KDiff3MergeEngine` doesn't share. Regression-tested via `DiffPlexMergeEngineTests`' `BuildMerge_InterleavedIndependentEdits_...`/`MergeHeadless_InterleavedIndependentEdits_...` fixtures (the minimal repro above). Do not "fix" this by switching chunkers - the bug reproduces under DiffPlex's own default/tested `LineChunker` too, just at a somewhat lower rate, so it isn't a `LineEndingsPreservingChunker`-specific problem and switching would trade a real, working byte-for-byte line-ending-preservation property for no actual safety gain. -- **Verify KDiff3 process behavior via `Process.Start(fileName, argsString)` (the two-string overload, `UseShellExecute=false` by default on modern .NET), not via a shell.** A prior verification pass tested KDiff3 invocation through Git Bash/MSYS2 and concluded `damageManagerProcessor.ws` (a second real conflict) still needed manual GUI resolution even after encoding normalization. Re-tested later through .NET's `Process.Start` — the actual code path this app uses — the same file (both raw and normalized) auto-solved cleanly every time; the bash-based test was an invocation-environment artifact, not real KDiff3 behavior. A guaranteed-genuine conflict (two synthetic mods editing the identical line differently) confirmed what actually distinguishes the two outcomes: KDiff3 always briefly shows a window titled exactly `Conflicts` on startup (auto-solves or not — this is not a "needs manual resolution" signal), but only a genuine unresolved conflict leaves a second window open whose title ends in `" - KDiff3"` (the actual comparison/merge editor, e.g. `Vanilla <-> modA <-> modB - KDiff3`) — that one persists indefinitely until closed, while an auto-solve's process exits within a few seconds regardless of file size (3400+ line files exited in under 3s in testing). Any headless/non-interactive invocation path must detect on window persistence past a short grace period (~2-3s, to let the transient `Conflicts` window close), not on elapsed time alone and not by assuming a visible window means failure. -- **KDiff3's pop-up window can't be suppressed without breaking the merge — don't try.** Five techniques were tested empirically (scratch harness in a session's `scratchpad/detector-test/`) against both an auto-solve case and a guaranteed-conflict case: `ProcessStartInfo.WindowStyle = Hidden` and `= Minimized` are both silently ignored by KDiff3/Qt (window shows full-size regardless, confirmed via `IsIconic` for the minimized case) but don't break anything; `ShowWindow(hwnd, SW_HIDE)`, `SetWindowPos` moved off-screen, and launching on a separate non-interactive Windows desktop (`CreateDesktop`) all three genuinely succeed at making the window invisible — and all three reliably make KDiff3 hang forever at its "Conflicts" splash instead of ever auto-solving (confirmed against a clean control: the identical launch mechanism, untouched, auto-solves in 1.6–6.5s every time). The pattern held across three independent suppression mechanisms, which is strong evidence KDiff3's Qt runtime needs the window genuinely composited on the real, interactive desktop to make progress at all — not something fixable from outside the process. The window does steal foreground focus while shown (confirmed via `GetForegroundWindow()`). Given that hard constraint, `KDiff3.RunHeadless` accepts the window appearing and instead attempts to restore focus to whatever had it beforehand (captured before launching KDiff3, restored in a `finally` once KDiff3's own window is confirmed gone — `proc.Kill` is async, so the kill path also waits up to 2s via `WaitForExit` before restoring, or the restore could race a still-alive window). **This restoration is unverified in practice — treat it as attempted, not guaranteed.** Plain `SetForegroundWindow` was empirically denied every time in testing (Windows' foreground-lock policy: WSM's own process never owned the foreground to begin with, since KDiff3's window did, so it isn't a privileged caller by the time it tries to restore). `RestoreForegroundWindow` upgrades to the standard `AttachThreadInput` workaround (temporarily share input state with whatever thread currently owns the foreground, then call `SetForegroundWindow`) — but this was *also* observed to be denied in every test run in this session's sandboxed automation environment. Whether that's a real limit or an artifact of that specific environment (its own automation harness aggressively reclaiming focus) is unresolved; it hasn't been tested from a normal interactive user session. Treat the restore as a good-faith best-effort mitigation, not a proven one, until someone verifies it from an ordinary desktop session. Do not reintroduce any of the three broken suppression techniques without re-verifying they still hang; if a future KDiff3 update changes this behavior, that verification needs to be redone before trusting a different result. -- **`RunHeadless`'s ~250ms detection-loop poll interval is load-bearing — don't tighten it.** Discovered by accident while testing the suppression techniques above: polling every 15ms for the first second — with *zero* window manipulation, just `EnumWindows`/`GetWindowText` read-only queries — reliably hung KDiff3 the same way the suppression techniques did; the identical untouched launch polled at 200ms auto-solved normally every time. `GetWindowText` issues a cross-process `SendMessage(WM_GETTEXT)` to the target window, which is a blocking call the target thread must service — a plausible mechanism is that polling fast enough starves or reorders KDiff3's own message loop during the window it needs to actually compute the merge. "Poll faster to detect the conflict window sooner" is a natural-looking optimization that would silently turn every merge into a hang — don't make this change without re-verifying against both an auto-solve case and a guaranteed-conflict case first. +- **Text-merge input encoding must stay normalized to UTF-16LE, never down to UTF-8.** Vanilla `.ws` files are UTF-16LE with a BOM; mod authors' files are often plain UTF-8/ASCII with no BOM (confirmed against real files on a live install). Mismatched encodings can make an entire file read as unmatchable against its counterpart, turning a false conflict into "needs manual resolution" that would otherwise have auto-solved cleanly — empirically confirmed real-world case: `baseEffect.ws` failed to auto-solve with mismatched encodings (against the now-retired KDiff3 engine, which had no per-input encoding flag at all - see `docs/decisions/kdiff3-retirement.md`) and succeeded cleanly once normalized, with correct merged output. `Tools/FileEncoding.cs` (Core) handles this: `ReadAnyEncoding`/`WriteUtf16` give `DiffPlexMergeEngine` UTF-16LE normalization without needing a temp file, since it merges in-process text directly (`EnsureUtf16File`, an on-disk-copy variant for a tool that needs a file path instead, has no in-repo caller since KDiff3's retirement but is kept, still unit-tested, for any future file-based tool) — never normalize toward UTF-8, since the game may not load a merged `.ws` file in that encoding. `ReadAnyEncoding` deliberately uses `File.ReadAllText(path)`'s built-in BOM auto-detection rather than decoding raw bytes with a fixed `Encoding` instance - the latter does not strip a detected BOM, leaving a stray U+FEFF glued to the first line, confirmed empirically to reproduce the exact `baseEffect.ws`-style false conflict this whole mechanism exists to avoid (see `WitcherScriptMerger.Tests`' `MergeHeadless_EncodingMismatch_...` fixture). +- **DiffPlex's `ThreeWayDiffer` (1.9.0) can produce internally inconsistent diff blocks - `DiffPlexMergeEngine.BuildMerge` must never trust its output without checking.** Confirmed as a genuine upstream library bug, not a defect in this repo's own merge loop: `BuildMerge`'s block-iteration loop is a faithful port of DiffPlex's own `ThreeWayDiffer.CreateMerge` (same index-chasing shape), and a throwaway scratch console app (per this repo's testing convention) calling DiffPlex's own `CreateMerge` directly - with both `LineChunker` (DiffPlex's own default, and the *only* chunker its own `Facts.DiffPlex/ThreeWayDifferFacts.cs` test suite ever exercises for 3-way diffs) and `LineEndingsPreservingChunker` (the one this engine actually uses) - reproduced the identical failure on the identical input either way. When old-side and new-side edits interleave/overlap relative to base in certain ways, `CreateThreeWayDiffBlocks` can emit a block list whose `OldCount`/`NewCount` don't actually correspond to the real `PiecesOld`/`PiecesNew` arrays. This surfaces two ways: an outright `ArgumentOutOfRangeException` from direct indexer access, or - confirmed via a minimal repro (base `"a();/b();/c();"`, one side inserts a line after `a()`, the other independently changes `b()` to `B()`) - no exception at all, but silently wrong output (content lost or duplicated), because the running `oldIndex`/`newIndex` end up not matching `PiecesOld.Count`/`PiecesNew.Count` even though no single block's own bookkeeping looked wrong in isolation. A large randomized stress test (varying edit density and file length, run against the real, fixed `BuildMerge`) measured combined failure rates of **0.35%** at one independent single-line edit per side on 50-200 line files (the closest analogue to a typical two-mod `.ws` conflict), rising to **0.88%** (1-2 edits/side), **2.65%** (2-3 edits/side), **4.99%** (1-6 edits/side on 50-200 line files), and **38.89%** on the original dense adversarial case (1-6 edits/side on 1-19 line files) - zero cases of any exception type other than the one this bug produces, across 100,000 total trials. `BuildMerge` defends against both failure modes: the block-processing loop is wrapped in `try`/`catch (ArgumentOutOfRangeException)`, and a post-loop check verifies `oldIndex`/`newIndex` actually reached `PiecesOld.Count`/`PiecesNew.Count` (accounting for a legitimate trailing-unchanged gap needing the exact same lockstep advance as the per-block gap-catchup above it - an early, incorrect version of this check that skipped that trailing advance produced a ~33% false-positive "inconsistent" rate on the exact same benign inputs). Either failure mode throws a `DiffPlexMergeEngine.DiffAlgorithmException`, which `MergeHeadless` catches and reports as `NeedsManualResolution` **without writing anything, including a conflict-marker sidecar** - the marker content itself would have been built from the same untrustworthy piece indices, so this is the one case where `DiffPlexMergeEngine` can't offer a conflict-marker starting point at all. This measured, non-negligible failure rate at realistic edit density is a real reliability gap the now-retired KDiff3 engine didn't share, and is the primary reason retiring KDiff3 was a deliberate tradeoff, not a strict improvement - see `docs/decisions/kdiff3-retirement.md`. Regression-tested via `DiffPlexMergeEngineTests`' `BuildMerge_InterleavedIndependentEdits_...`/`MergeHeadless_InterleavedIndependentEdits_...` fixtures (the minimal repro above). Do not "fix" this by switching chunkers - the bug reproduces under DiffPlex's own default/tested `LineChunker` too, just at a somewhat lower rate, so it isn't a `LineEndingsPreservingChunker`-specific problem and switching would trade a real, working byte-for-byte line-ending-preservation property for no actual safety gain. +- **KDiff3's empirically-discovered process behavior (window-title polling, poll-interval sensitivity, failed suppression attempts, unverified focus restoration) is preserved in `docs/decisions/kdiff3-retirement.md`, not here.** That code (`Tools/KDiff3.cs`, `Tools/KDiff3MergeEngine.cs`) is deleted; the findings remain relevant only as a warning against reintroducing a similar external-GUI-tool integration without re-deriving them. ### Settings & persistence @@ -117,12 +121,13 @@ The CLI path (`FileMerger.MergeConflictsHeadless`) is a separate, parallel orche ### External tool dependencies -Three bundled Windows executables are invoked via `Process.Start`, with relative paths configured in `App.config`'s `` (`KDiff3Path`, `QuickBmsPath`, `QuickBmsPluginPath`, `WccLitePath`): -- **KDiff3** (`Tools\KDiff3\KDiff3.exe`) — GPL-licensed, safe to bundle into a release. +Two bundled Windows executables are invoked via `Process.Start`, with relative paths configured in `App.config`'s `` (`QuickBmsPath`, `QuickBmsPluginPath`, `WccLitePath`): - **QuickBMS** (`Tools\QuickBMS\quickbms.exe` + `witcher3.bms` plugin) — no license file found; do not add to source control. - **wcc_lite** (`Tools\wcc_lite\bin\x64\wcc_lite.exe`) — no license file found; do not add to source control. -None of these binaries are committed to this repo (matches the original upstream project's precedent) — keep it that way; if packaging is tackled later, it belongs in a separate release artifact, not source control. +Neither binary is committed to this repo (matches the original upstream project's precedent) — keep it that way; if packaging is tackled later, it belongs in a separate release artifact, not source control. + +KDiff3 (`Tools\KDiff3\KDiff3.exe`, GPL-licensed, safe to bundle into a release) used to be a third dependency here, for text merges — it was retired in favor of an in-process DiffPlex-based engine that needs no external binary at all; see `docs/decisions/kdiff3-retirement.md`. ## Coding standards & SOP diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index f8958c9..742f29a 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -19,14 +19,14 @@ Match the existing source (e.g. `Inventory/FileMerger.cs`, `Controls/SMTree.cs`) - **`main` is protected.** No direct commits or pushes — all changes land via pull request. Force-pushes and branch deletion are disabled on `main` at the GitHub level. - **Branch per feature/fix**, off `main`: `feature/` for new functionality, `fix/` for bug fixes, `chore/` for tooling/process/docs changes not tied to a feature or bug. Keep the description short and kebab-case (e.g. `fix/kdiff3-encoding-mismatch`). - **Pull requests require 2 approving reviews** before merge (GitHub branch protection on `main`). This applies to everyone, including repository admins in normal circumstances — admin bypass exists at the platform level for genuine emergencies, not as a routine shortcut. -- **PR description should cover**: what changed and why, and — given there's no test suite (see Testing below) — specifically *how you verified it*. "Builds successfully" is necessary but not sufficient for anything touching hash output, `MergeInventory.xml` schema, KDiff3/QuickBMS/wcc_lite invocation, or encoding handling; see `CLAUDE.md`'s Compatibility constraints for why those are load-bearing, and its Tests section for the verification pattern this codebase uses in place of a test suite. +- **PR description should cover**: what changed and why, and — given there's no test suite (see Testing below) — specifically *how you verified it*. "Builds successfully" is necessary but not sufficient for anything touching hash output, `MergeInventory.xml` schema, QuickBMS/wcc_lite invocation, the DiffPlex-based merge engine, or encoding handling; see `CLAUDE.md`'s Compatibility constraints for why those are load-bearing, and its Tests section for the verification pattern this codebase uses in place of a test suite. - Commit messages are short, descriptive sentences (e.g. `Fixed crash after canceling file-open.`, `Replace hand-ported xxHash32 with System.IO.Hashing`). A `Category:` prefix (`Fixed:`, etc.) shows up occasionally but isn't enforced. No Conventional Commits format required. - GitHub Actions CI (`.github/workflows/build.yml`) runs `dotnet build --configuration Release` and `dotnet format whitespace --verify-no-changes` on every PR targeting `main`, but don't rely on it to catch problems for you — run both locally first: `dotnet build WitcherScriptMerger.sln --configuration Release` and `dotnet format whitespace WitcherScriptMerger.sln --verify-no-changes` before opening a PR. Catching failures before CI does saves a round trip. -- External binary dependencies (KDiff3, QuickBMS, wcc_lite — see `CLAUDE.md`'s External tool dependencies) aren't in source control, so a fresh clone needs them sourced separately before the app runs end-to-end. PRs that only touch code not exercising those tools don't need them to build and review. +- External binary dependencies (QuickBMS, wcc_lite — see `CLAUDE.md`'s External tool dependencies) aren't in source control, so a fresh clone needs them sourced separately before the app runs end-to-end. PRs that only touch code not exercising those tools don't need them to build and review. (KDiff3 used to be a third such dependency; it was retired — see `docs/decisions/kdiff3-retirement.md`.) ## Testing -There's no test project in this repo. For changes that touch hash output, `MergeInventory.xml` schema, or KDiff3 invocation, use a disposable, non-committed scratch console app: exercise synthetic edge cases plus a cross-check against a real value already recorded in a live `MergeInventory.xml`. See `CLAUDE.md`'s Tests section for the specifics of why this matters for this codebase. Describe what you actually ran in your PR description — see Repository SOP above. +`WitcherScriptMerger.Tests` (xunit) covers `WitcherScriptMerger.Core` — see `CLAUDE.md`'s Tests section for what it covers and its constraints. For anything not covered there — especially further hash output or `MergeInventory.xml` schema changes — use a disposable, non-committed scratch console app: exercise synthetic edge cases plus a cross-check against a real value already recorded in a live `MergeInventory.xml`. Describe what you actually ran in your PR description — see Repository SOP above. ## AI-assisted development @@ -34,7 +34,7 @@ This repository is developed with AI coding agents (Claude Code, and expect othe - **Disclose it.** If a PR was substantially produced or assisted by an AI coding agent, say so in the PR description. Commits already carry a `Co-Authored-By` trailer when an agent is involved (Claude Code does this automatically) — that's necessary but not sufficient; the PR description is where a reviewer looks first. - **You own what you submit, regardless of how it was produced.** Be able to explain any part of your own PR if a reviewer asks — "the agent wrote it that way" isn't an answer to "why does this work." If you can't explain a change, that's a signal to understand it better before submitting, not to submit it anyway. -- **The verification bar doesn't move for AI-assisted changes — if anything, hold it higher.** This codebase has no test suite and several genuinely load-bearing, non-obvious compatibility constraints (hash format, KDiff3 encoding normalization, the window-persistence detection in headless mode — all documented in `CLAUDE.md`). Agents are good at producing code that looks plausible and compiles; they have no way to know these constraints exist unless `CLAUDE.md` tells them, and no way to know their fix actually works unless it's actually run against real data. "Should work" is not verification — see Testing above. +- **The verification bar doesn't move for AI-assisted changes — if anything, hold it higher.** This codebase has a thin, Core-only test suite and several genuinely load-bearing, non-obvious compatibility constraints (hash format, text-merge input encoding normalization, the DiffPlex upstream bug `DiffPlexMergeEngine` has to defend against on every merge — all documented in `CLAUDE.md`). Agents are good at producing code that looks plausible and compiles; they have no way to know these constraints exist unless `CLAUDE.md` tells them, and no way to know their fix actually works unless it's actually run against real data. "Should work" is not verification — see Testing above. - **Scrub machine-specific state before submitting.** Agent-assisted sessions tend to accumulate absolute local paths, scratch config pointing at a personal install, or test artifacts from the working process — check your diff for anything like a `G:\SteamLibrary\...`-style path or a personal game install location before opening a PR. `.gitignore` excludes common agent runtime-state directories (`.claude/`, `.cursor/`, etc.) and session handoff notes (`HANDOFF*.md`) for the same reason — extend it rather than working around it if your tool of choice uses a different local-state convention. - **You're responsible for license compatibility of anything an agent produces**, same as for hand-written code — this project cares about this already (see `CLAUDE.md`'s External tool dependencies section on why QuickBMS/wcc_lite specifically aren't bundled). Don't accept agent output that reproduces code from a source with an incompatible license. - **Bulk or automated PRs still go through the same process.** A large refactor being agent-generated isn't a reason to skip branch-per-change, PR review, or the two-approval requirement — if anything, larger diffs benefit more from review, not less. diff --git a/README.md b/README.md index 9481c43..9f24258 100644 --- a/README.md +++ b/README.md @@ -3,8 +3,8 @@ I threw together this tool because I got tired of manually merging script files. - Checks your Mods folder for mod conflicts. Uses [QuickBMS](http://aluigi.altervista.org/quickbms.htm) to scan .bundle packages. -- Merges .ws scripts or .xml files inside bundle packages using the powerful open-source merge tool [KDiff3](http://kdiff3.sourceforge.net/). +- Merges .ws scripts or .xml files inside bundle packages using an in-process 3-way merge engine built on [DiffPlex](https://github.com/mmanela/diffplex) — no external merge tool required. A conflict that can't be auto-solved is written to a conflict-marker file and opened for manual review instead. (This fork previously used the external tool KDiff3 for this; see `docs/decisions/kdiff3-retirement.md` for why it was retired.) - Packages new .bundle packages using the official mod tool [wcc_lite](http://www.nexusmods.com/witcher3/news/12625/?). - Detects updated merge source files using the [xxHash](https://github.com/Cyan4973/xxHash) algorithm by Yann Collet, [implemented in .NET](https://github.com/wilhelmliao/xxHash.NET) by Wilhelm Liao. -**KDiff3 & other external binary dependencies aren't included in this source code.** +**QuickBMS & wcc_lite aren't included in this source code.** diff --git a/WitcherScriptMerger.Core/AppState.cs b/WitcherScriptMerger.Core/AppState.cs index e0c673c..d0c671a 100644 --- a/WitcherScriptMerger.Core/AppState.cs +++ b/WitcherScriptMerger.Core/AppState.cs @@ -1,7 +1,6 @@ using System.Threading; using WitcherScriptMerger.Inventory; using WitcherScriptMerger.LoadOrder; -using WitcherScriptMerger.Tools; namespace WitcherScriptMerger { @@ -71,10 +70,6 @@ public static AppSettings Settings public static CustomLoadOrder LoadOrder = null; public static MergeInventory Inventory = null; - // Set once by the host project at startup (see Program.cs) to a - // KDiff3MergeEngine - see Tools/IMergeEngine.cs for why this exists. - public static IMergeEngine MergeEngine = null; - static AppState() { } } } diff --git a/WitcherScriptMerger.Core/Cli/MergeOperations.cs b/WitcherScriptMerger.Core/Cli/MergeOperations.cs index 88d52bd..ab9be28 100644 --- a/WitcherScriptMerger.Core/Cli/MergeOperations.cs +++ b/WitcherScriptMerger.Core/Cli/MergeOperations.cs @@ -32,10 +32,7 @@ public static FileMerger.HeadlessMergeSummary RunMerge( IReadOnlyDictionary orderOverrides, bool dryRun = false) { - // AppState.MergeEngine is supplied once by the host project at startup - // (Program.cs) - see Tools/IMergeEngine.cs for why Core can't construct - // its one real implementation (KDiff3MergeEngine) itself. - var merger = new FileMerger(inventory, AppState.MergeEngine); + var merger = new FileMerger(inventory); return merger.MergeConflictsHeadless(conflicts, mergedModName, orderOverrides, dryRun); } } diff --git a/WitcherScriptMerger.Core/Inventory/FileMerger.cs b/WitcherScriptMerger.Core/Inventory/FileMerger.cs index 20ae1ae..07aef66 100644 --- a/WitcherScriptMerger.Core/Inventory/FileMerger.cs +++ b/WitcherScriptMerger.Core/Inventory/FileMerger.cs @@ -27,8 +27,9 @@ namespace WitcherScriptMerger.Inventory // InteractiveMergeRunner supplies callbacks that build the real forms, call // MainForm.ShowModal, and play the sound - all exactly where the old inline // `using (var reportForm = ...) { ShowModal }` blocks used to run. - // - KDiff3 invocation goes through IMergeEngine instead of calling Tools/KDiff3.cs - // directly - see Tools/IMergeEngine.cs for why. + // - Text-merge invocation goes through Tools/DiffPlexMergeEngine.cs directly - KDiff3 + // (and the IMergeEngine interface that used to sit between this class and it) was + // retired; see docs/decisions/kdiff3-retirement.md. public class FileMerger { #region Types @@ -99,8 +100,6 @@ public class MergeReportData public MergeProgressInfo ProgressInfo { get; private set; } - public IMergeEngine MergeEngine { get; set; } - // Invoked after a successful interactive merge/bundle pack. Only ever set (and // only ever invoked) on the interactive path - MergeConflictsHeadless never // touches these. See InteractiveMergeRunner.cs for what the host project's @@ -113,28 +112,22 @@ public class MergeReportData string _mergedModName; string _outputPath; + // The sole text-merge engine (see DiffPlexMergeEngine.cs's own header comment for + // why this is a direct field rather than an injected IMergeEngine - that interface + // was deleted along with KDiff3MergeEngine, its only other implementation). Not + // exposed as a public property: nothing outside this class has ever needed to read + // or replace it, unlike when this was an injected dependency selected once at + // startup (Program.Main used to choose between two implementations here). + DiffPlexMergeEngine _mergeEngine = new DiffPlexMergeEngine(); + bool _bundleChanged; List _pendingBundleMerges = new List(); #endregion - public FileMerger(MergeInventory inventory, IMergeEngine mergeEngine) + public FileMerger(MergeInventory inventory) { - // AppState.MergeEngine (the usual source callers pass here) defaults to - // null and is only ever populated by the one real entry point - // (Program.Main, before anything else runs) - nothing in the type system - // enforces that. Failing fast here with a clear message beats letting - // Merge()/MergeHeadless() throw an unhandled NullReferenceException from - // deep inside a merge the first time any future entry point (a test - // harness, the Linux CLI/MCP-only host planned for a later unit) - // constructs a FileMerger without going through that startup path first. - if (mergeEngine == null) - throw new ArgumentNullException(nameof(mergeEngine), - "FileMerger requires a non-null IMergeEngine. If this was constructed via " + - "AppState.MergeEngine, the host entry point never set it - see Tools/IMergeEngine.cs."); - _inventory = inventory; - MergeEngine = mergeEngine; ProgressInfo = new MergeProgressInfo(); } @@ -274,13 +267,12 @@ void MergeBundleFileInteractive(InteractiveMergeRequest file, Merge merge, bool FileInfo MergeTextInteractive(Merge merge, MergeSource source1, MergeSource source2) { - // Deliberately engine-neutral wording: this used to name KDiff3 explicitly - // ("waiting for KDiff3 to close"), which is wrong when MergeEngine is - // DiffPlexMergeEngine instead - no external process or window is involved - // there at all. Flagged in code review, see CLAUDE.md. + // Deliberately engine-neutral wording rather than naming KDiff3 explicitly + // ("waiting for KDiff3 to close") - no external process or window is involved + // at all with DiffPlexMergeEngine. ProgressInfo.CurrentAction = $"Merging {source1.Name} && {source2.Name}"; - var result = MergeEngine.Merge(source1, source2, _vanillaFile, _outputPath); + var result = _mergeEngine.Merge(source1, source2, _vanillaFile, _outputPath); if (result != MergeEngineResult.AutoSolved) return null; @@ -611,7 +603,14 @@ FileInfo MergeTextHeadless(Merge merge, MergeSource source1, MergeSource source2 { ProgressInfo.CurrentAction = $"Merging {source1.Name} && {source2.Name}"; - var result = MergeEngine.MergeHeadless(source1, source2, _vanillaFile, _outputPath); + // openConflictMarkers: false for a dry run - a genuine conflict still writes + // its conflict-marker sidecar (pre-existing behavior; see + // DiffPlexMergeEngine.MergeHeadless's own comment), but must not launch a real + // editor/process for what's supposed to be a side-effect-free preview. Without + // this, MergeConflictsHeadless(dryRun: true) against a mods folder with N + // genuine conflicts would pop open N editor windows - a real bug caught in + // review before it shipped (see docs/decisions/kdiff3-retirement.md). + var result = _mergeEngine.MergeHeadless(source1, source2, _vanillaFile, _outputPath, openConflictMarkers: !dryRun); if (result != MergeEngineResult.AutoSolved) return null; diff --git a/WitcherScriptMerger.Core/Mcp/WsmMcpTools.cs b/WitcherScriptMerger.Core/Mcp/WsmMcpTools.cs index 16a3bf0..1ef3285 100644 --- a/WitcherScriptMerger.Core/Mcp/WsmMcpTools.cs +++ b/WitcherScriptMerger.Core/Mcp/WsmMcpTools.cs @@ -48,8 +48,9 @@ public static object ScanConflicts() } [McpServerTool(Name = "merge_conflicts"), Description( - "Merges detected conflicts headlessly - never opens KDiff3's GUI; conflicts that can't " + - "be auto-solved are skipped and reported, not merged. Restrict to specific files with " + + "Merges detected conflicts headlessly; conflicts that can't be auto-solved are skipped " + + "and reported, not merged - a conflict-marker sidecar file is written and opened in the " + + "default editor for manual review instead. Restrict to specific files with " + "relativePaths (default: every detected conflict); override a file's mod merge order " + "with orderOverrides (default merge order otherwise matches the game's own load order). " + "Set dryRun to preview which conflicts would auto-solve without writing any merged " + @@ -140,7 +141,7 @@ public static object MergeConflicts( [McpServerTool(Name = "get_status"), Description( "Reports WSM's current configuration and dependency status: resolved game/mods " + - "directories, whether KDiff3/QuickBMS/wcc_lite are all found, the configured " + + "directories, whether QuickBMS/wcc_lite are all found, the configured " + "merged-mod name, and the current conflict count.")] public static object GetStatus() { @@ -181,7 +182,7 @@ static void RequireDependenciesAndModsDirectory() { if (!Paths.ValidateDependencyPaths()) throw new InvalidOperationException( - "A required dependency (KDiff3, QuickBMS, or wcc_lite) is missing. Configure its path in App.config."); + "A required dependency (QuickBMS or wcc_lite) is missing. Configure its path in App.config."); if (!Directory.Exists(Paths.ModsDirectory)) throw new InvalidOperationException("Mods directory not found - check GameDirectory/ModsDirectory in App.config."); diff --git a/WitcherScriptMerger.Core/Paths.cs b/WitcherScriptMerger.Core/Paths.cs index 1f38c36..f0f5f9a 100644 --- a/WitcherScriptMerger.Core/Paths.cs +++ b/WitcherScriptMerger.Core/Paths.cs @@ -82,19 +82,13 @@ public static string GetRelativePath(string fullPath, string basePath) return fullPath.Substring(startIndex); } - // KDiff3's own exe-path check goes through AppState.MergeEngine rather than a - // direct reference to Tools/KDiff3.cs, which stays in the host project for - // its Win32 P/Invoke and so can't be referenced from Core - see - // Tools/IMergeEngine.cs. Like AppState.Notifier/Settings, this relies on the - // host having set AppState.MergeEngine before calling in - true for the one - // real entry point (Program.Main, first line) but not enforced by the type - // system; a null MergeEngine here reads as "dependency missing" rather than - // "not initialized yet", which could be a confusing message if that - // invariant is ever broken by a future entry point. + // KDiff3 used to be a third dependency validated here (via AppState.MergeEngine - + // see docs/decisions/kdiff3-retirement.md); now that DiffPlexMergeEngine is the + // sole text-merge engine and needs no external binary at all, only QuickBMS/wcc_lite + // remain real external-tool dependencies. public static bool ValidateDependencyPaths() { - return (AppState.MergeEngine != null && AppState.MergeEngine.ValidateExePath() && - File.Exists(QuickBms.ExePath) && + return (File.Exists(QuickBms.ExePath) && File.Exists(QuickBms.PluginPath) && File.Exists(WccLite.ExePath)); } diff --git a/WitcherScriptMerger.Core/Tools/DiffPlexMergeEngine.cs b/WitcherScriptMerger.Core/Tools/DiffPlexMergeEngine.cs index 00d73da..ab69b46 100644 --- a/WitcherScriptMerger.Core/Tools/DiffPlexMergeEngine.cs +++ b/WitcherScriptMerger.Core/Tools/DiffPlexMergeEngine.cs @@ -12,24 +12,40 @@ namespace WitcherScriptMerger.Tools { - // In-process, external-binary-free alternative to KDiff3MergeEngine (host project), - // built on DiffPlex (MIT-licensed NuGet package)'s ThreeWayDiffer. Not the active - // engine by default - see Program.Main (host project) for the "MergeEngine" App.config - // switch. See Tools/IMergeEngine.cs for why this interface exists at all: it's - // Core/host split scaffolding, not a permanent pluggable-engine abstraction, and a - // later unit that removes KDiff3 entirely may delete the interface and inline this - // engine's logic directly into FileMerger. + // Return type for DiffPlexMergeEngine.Merge/MergeHeadless. Used to live in a deleted + // IMergeEngine interface alongside a since-retired KDiff3MergeEngine (see + // docs/decisions/kdiff3-retirement.md) - kept as its own small enum, rather than + // deleted along with the interface, since FileMerger and this class's own test suite + // both consume it as a stable return-type vocabulary independent of any interface. + public enum MergeEngineResult + { + AutoSolved, + NeedsManualResolution, + Failed, + } + + // The sole text-merge engine - in-process, external-binary-free, built on DiffPlex + // (MIT-licensed NuGet package)'s ThreeWayDiffer. KDiff3 (formerly the default, and + // before that the only, text-merge engine) was retired; see + // docs/decisions/kdiff3-retirement.md for the full rationale and for the empirical + // KDiff3 process-behavior findings preserved there now that KDiff3.cs itself is gone. + // + // FileMerger (Core) constructs and calls this class directly - there's no more + // IMergeEngine interface indirection. That interface existed only so Core could reach + // a text-merge engine without referencing Tools/KDiff3.cs's Win32 P/Invoke, which had + // to stay in the host project; now that KDiff3MergeEngine is gone, DiffPlexMergeEngine + // (already Core-side, same as FileMerger) is the only implementation there will ever + // be, so the interface no longer bridges anything - keeping it would be exactly the + // premature "abstraction with one implementation for its whole remaining life" this + // repo's own conventions say to avoid (see CLAUDE.md/CONTRIBUTING.md). // - // There's no UI here at all - unlike KDiff3MergeEngine, which can open KDiff3's own - // GUI for the interactive path - so "interactive" and "headless" collapse to the same - // underlying logic. Merge() just runs MergeHeadless() and maps NeedsManualResolution - // to Failed, since IMergeEngine.Merge's contract explicitly forbids ever returning - // NeedsManualResolution (that's a headless-only concept - see the interface's doc - // comment). One real behavior difference from KDiff3MergeEngine as a result: the - // "ReviewEachMerge" setting (show the merge UI even for auto-solvable merges, so the - // user can double check it) has nothing to open here and is silently not honored - - // there is no in-process equivalent to implement it against. - public class DiffPlexMergeEngine : IMergeEngine + // There's no UI here at all, so "interactive" and "headless" collapse to the same + // underlying logic: Merge() just runs MergeHeadless() and maps NeedsManualResolution + // to Failed, since Merge's contract forbids ever returning NeedsManualResolution + // (that's a headless-only concept). This also means a genuine conflict's + // conflict-marker sidecar gets written and opened (see MergeHeadless below) on the + // interactive path too, since it's the exact same code path underneath. + public class DiffPlexMergeEngine { #region Types @@ -88,11 +104,25 @@ public MergeEngineResult Merge( return result == MergeEngineResult.NeedsManualResolution ? MergeEngineResult.Failed : result; } + // openConflictMarkers defaults to true (matching Merge()'s interactive-path + // behavior, and every pre-existing headless caller) - FileMerger.MergeTextHeadless + // is the one caller that passes false, for a dry run (see MergeConflictsHeadless's + // dryRun parameter): a dry run's whole contract is "preview only, no side effects + // a user didn't ask for" (the MCP merge_conflicts tool's own dryRun description + // promises no merged output, bundle repack, or MergeInventory.xml write), and + // FileOpener.Open launching a real editor/process is exactly that kind of surprise + // side effect for an operation whose entire point is to be inspectable without + // consequence. The conflict-marker sidecar itself is still written either way + // (pre-existing behavior, not something this parameter changes) - only the + // auto-open is conditional, since that's the specific side effect that turns a + // preview into something visibly disruptive (an editor window popping up per + // conflict for a mods folder with many of them). public MergeEngineResult MergeHeadless( FileMerger.MergeSource source1, FileMerger.MergeSource source2, FileInfo vanillaFile, - string outputPath) + string outputPath, + bool openConflictMarkers = true) { var hasVanillaVersion = vanillaFile != null && vanillaFile.Exists; @@ -109,16 +139,14 @@ public MergeEngineResult MergeHeadless( // GetUnpackedFiles leaves _vanillaFile null), but this guard applies // unconditionally to any conflict with no vanilla file, flat or bundled - // safest, and consistent with HeadlessMergeNotifier's non-destructive - // defaults, is to refuse rather than guess. Note this is a real, deliberate - // behavior difference from KDiff3MergeEngine: Tools/KDiff3.cs's BuildArgs has - // no equivalent guard and always attempts a real (if degraded, vanilla-less) + // defaults, is to refuse rather than guess. The now-retired KDiff3 engine had + // no equivalent guard - it always attempted a real (if degraded, vanilla-less) // 2-way --auto merge in this situation instead of refusing outright, because - // KDiff3 itself has a coherent notion of a 2-file diff/merge - DiffPlex's - // ThreeWayDiffer, as used here, does not, so there's no equally meaningful - // fallback to attempt. Which conflicts even get attempted can therefore differ - // depending on which engine is configured; flagged in code review, not fixed - // by building a parallel 2-way DiffPlex merge path since that's new scope - // beyond what this engine set out to replicate - see CLAUDE.md. + // KDiff3 itself had a coherent notion of a 2-file diff/merge that DiffPlex's + // ThreeWayDiffer, as used here, does not, so there was no equally meaningful + // fallback to attempt (see docs/decisions/kdiff3-retirement.md). Not fixed by + // building a parallel 2-way DiffPlex merge path here since that's new scope + // beyond what this engine set out to replicate. if (!hasVanillaVersion) { AppState.Notifier.ShowMessage( @@ -128,20 +156,16 @@ public MergeEngineResult MergeHeadless( return MergeEngineResult.NeedsManualResolution; } - // Same "merging an updated mod file into an existing merge chain" guard - // KDiff3.RunHeadless applies (see its comment for the full reasoning) - kept - // duplicated here rather than hoisted into FileMerger since that's shared - // orchestration code outside this unit's scope; a later unit collapsing the - // merge engines should consider moving it there instead of keeping two copies. - // One real consequence of the duplication (vs. hoisting into FileMerger, - // which both Merge and MergeHeadless funnel through) worth calling out: since - // Merge() (the interactive path) just delegates straight to MergeHeadless() - // here (see this class's header comment - there's no UI to fall back to), - // this outdated-hash case comes back as Failed on the interactive path too, - // where KDiff3MergeEngine's own interactive Run() instead opens KDiff3's GUI - // for manual review. That gap already exists for every other kind of conflict - // on the DiffPlex interactive path (no UI here at all yet), so it isn't a new - // asymmetry this guard introduces - flagged in code review, see CLAUDE.md. + // The now-retired KDiff3.RunHeadless applied this same "merging an updated mod + // file into an existing merge chain" guard (see docs/decisions/kdiff3-retirement.md + // for its reasoning, preserved there since the code that motivated it is gone). + // Kept here rather than hoisted into FileMerger since that's shared + // orchestration code outside this class's scope - a future change collapsing + // this further could consider moving it there instead. Since Merge() (the + // interactive path) just delegates straight to MergeHeadless() (see this + // class's header comment - there's no UI to fall back to), this outdated-hash + // case comes back as Failed on the interactive path too, same as every other + // kind of conflict on this engine's interactive path. if (source1.TextFile.FullName.EqualsIgnoreCase(outputPath) && source2.Hash != null && source2.Hash.IsOutdated) { @@ -168,14 +192,15 @@ public MergeEngineResult MergeHeadless( // (see BuildMerge's comment) - don't write anything, including a sidecar: // the "conflict marker" content itself would have been built from the // same inconsistent piece indices, so it can't be trusted either. This is - // the one case where DiffPlexMergeEngine can't even offer a conflict-marker - // starting point the way KDiff3 always can - genuinely needs the source - // files opened side by side. + // the one case where DiffPlexMergeEngine can't offer a conflict-marker + // starting point at all - genuinely needs the source files opened side by + // side and compared by hand. AppState.Notifier.ShowMessage( $"Skipped {source1.Name} + {source2.Name}: the automatic 3-way merge algorithm hit " + $"an internal inconsistency it couldn't safely recover from ({ex.Message}) - a known " + "limitation of the underlying DiffPlex library for certain multi-edit conflicts, see " + - "CLAUDE.md. Needs manual resolution (e.g. via KDiff3).", + "CLAUDE.md. Needs manual resolution - open the source mod files directly to compare " + + "and resolve.", "Skipped", NotifyButtons.OK, DialogIcon.Warning); return MergeEngineResult.NeedsManualResolution; } @@ -202,20 +227,50 @@ public MergeEngineResult MergeHeadless( // (see GetConflictMarkerPath) keeps outputPath itself untouched (so retries // behave exactly as if this merge had never been attempted) while still // producing well-formed conflict-marker output at a predictable, computable - // location for a later unit to open in the user's default text editor. - FileEncoding.WriteUtf16(GetConflictMarkerPath(outputPath), result.MergedText); - + // location this method opens in the user's default editor below. + var sidecarPath = GetConflictMarkerPath(outputPath); + FileEncoding.WriteUtf16(sidecarPath, result.MergedText); + + // Path.GetFullPath here (message text only - the FileOpener.Open call below + // still passes sidecarPath as-is) since Paths.DiffPlexConflictsDirectory is + // relative (resolved against Environment.CurrentDirectory - see this class's + // header comment / CLAUDE.md's "Interactive vs. headless split" section for + // when that's pinned vs. not). A relative path in a user-facing message that + // might be the only record of where to find a conflict is close to useless if + // they read it later from a different working directory than the one that + // wrote it - the absolute form is unambiguous regardless of when/where it's read. + var sidecarFullPath = Path.GetFullPath(sidecarPath); + var openSuffix = openConflictMarkers + ? " - attempting to open it now for review." + : " - not opened automatically (dry run preview)."; AppState.Notifier.ShowMessage( $"Skipped {source1.Name} + {source2.Name}: genuine conflict, needs manual resolution. " + - $"Conflict markers were written to {GetConflictMarkerPath(outputPath)} for review.", + $"Conflict markers were written to {sidecarFullPath}{openSuffix}", "Skipped", NotifyButtons.OK, DialogIcon.Warning); + + // Runs after the notifier message (which blocks on the GUI's interactive + // path, via a real modal MessageBox - not here, headless) so the two read as + // one coherent sequence: acknowledge the skip, then the editor opens. Fires + // identically whether this method was reached via the CLI/MCP headless path + // directly or via the GUI's interactive Merge(), which just delegates to this + // method (see this class's header comment) - one mechanism for both, not two. + // Best-effort: FileOpener.Open never throws, and a failed open (no file + // association, etc.) doesn't change the result below - the sidecar is on disk + // either way. Skipped entirely for a dry run (openConflictMarkers = false) - + // see this method's parameter comment. + if (openConflictMarkers) + FileOpener.Open(sidecarPath); + return MergeEngineResult.NeedsManualResolution; } // No external executable - DiffPlex is an in-process managed library, so there's - // nothing to validate a path for. Note this doesn't remove QuickBMS/wcc_lite from - // Paths.ValidateDependencyPaths()'s checks - those are still required for bundle - // content regardless of which text-merge engine is active. + // nothing to validate a path for. No longer called from + // Paths.ValidateDependencyPaths() (that call site was removed along with + // IMergeEngine - QuickBMS/wcc_lite are still checked there directly, and remain + // required regardless of the text-merge engine); kept here since it's directly + // unit-tested and a trivially-true predicate costs nothing to keep around for any + // future caller. public bool ValidateExePath() => true; // Where a conflict-marker file is written when a merge can't be auto-solved - @@ -246,7 +301,8 @@ public MergeEngineResult MergeHeadless( // string.GetHashCode() was deliberately not used here - .NET randomizes string // hash codes per process by default, so it isn't stable across runs, unlike // XxHash32. This is still a computable, not merely a discoverable-by-browsing, - // location: a later unit wiring up "open in editor" can call this same method. + // location: MergeHeadless calls this same method right before opening the file + // (via FileOpener) for the exact path it just wrote. public static string GetConflictMarkerPath(string outputPath) { var pathHash = XxHash32.HashToUInt32(Encoding.UTF8.GetBytes(outputPath), 0); @@ -277,16 +333,17 @@ static void DeleteIfExists(string path) // - Purely-whitespace-only conflicts auto-resolve instead of producing markers, // mirroring KDiff3's --cs "WhiteSpace3FileMergeDefault=2" (verified against the // KDiff3 source: value 2 means "always pick input B", which is oldText/oldLabel - // here, matching KDiff3.BuildArgs' own file order of vanilla/source1/source2 - - // see CLAUDE.md's KDiff3 compatibility notes). + // here, matching the now-retired KDiff3 engine's own file order of + // vanilla/source1/source2 - see docs/decisions/kdiff3-retirement.md). // - Genuine conflicts are rendered as git/diff3-style conflict markers labeled // with the actual mod names, not DiffPlex's generic "old"/"base"/"new". // Uses LineEndingsPreservingChunker (not DiffPlex's default LineChunker) so // unchanged/single-side-changed content round-trips through unmodified, keeping // each such line's original line-ending byte-for-byte - only synthetic content // this method itself adds (conflict marker lines) uses an explicit "\r\n" to - // match vanilla .ws files' own DOS line endings (KDiff3.BuildArgs' own - // --cs "LineEndStyle=1" - confirmed against the KDiff3 source, value 1 is DOS). + // match vanilla .ws files' own DOS line endings (the now-retired KDiff3 engine's + // own --cs "LineEndStyle=1" - confirmed against the KDiff3 source, value 1 is DOS; + // see docs/decisions/kdiff3-retirement.md). public static MergeTextResult BuildMerge(string baseText, string oldText, string newText, string oldLabel, string newLabel) { // ThreeWayDiffer.CreateDiffs throws its own ArgumentNullException for a null diff --git a/WitcherScriptMerger.Core/Tools/FileEncoding.cs b/WitcherScriptMerger.Core/Tools/FileEncoding.cs index 4b62f93..fc5c641 100644 --- a/WitcherScriptMerger.Core/Tools/FileEncoding.cs +++ b/WitcherScriptMerger.Core/Tools/FileEncoding.cs @@ -3,19 +3,19 @@ namespace WitcherScriptMerger.Tools { - // Shared UTF-16LE+BOM normalization, used by every merge engine. KDiff3MergeEngine - // (host project) needs an on-disk temp copy since it shells out to an external exe - // that only accepts file paths; DiffPlexMergeEngine (Core) merges in-process and only - // needs the text itself, via ReadAnyEncoding/WriteUtf16 below - EnsureUtf16File exists - // for the former, kept here too so any future file-based tool can reuse it instead of - // duplicating this logic (this method used to be a private copy inside - // WitcherScriptMerger/Tools/KDiff3.cs::EnsureUtf16Encoding). + // Shared UTF-16LE+BOM normalization. DiffPlexMergeEngine merges in-process and only + // needs the text itself, via ReadAnyEncoding/WriteUtf16 below; EnsureUtf16File (an + // on-disk temp copy, for a tool that has to be handed a file path rather than raw + // text) has no in-repo caller since KDiff3MergeEngine's retirement (see + // docs/decisions/kdiff3-retirement.md - this method used to be a private copy inside + // WitcherScriptMerger/Tools/KDiff3.cs::EnsureUtf16Encoding) but is kept, and still + // directly unit-tested, for any future file-based tool that needs it. // // Vanilla .ws files are UTF-16LE with a BOM; mod authors' files are often plain // UTF-8/ASCII with no BOM (confirmed against real files on a live install) - see - // CLAUDE.md's "KDiff3 input encoding" compatibility constraint for why normalizing UP - // to UTF-16LE (never down to UTF-8) matters: the game may not load a merged .ws file - // that isn't UTF-16LE. + // CLAUDE.md's "Text-merge input encoding" compatibility constraint for why normalizing + // UP to UTF-16LE (never down to UTF-8) matters: the game may not load a merged .ws + // file that isn't UTF-16LE. public static class FileEncoding { // UTF-16LE with BOM - matches vanilla .ws file encoding. Never normalize merge @@ -71,9 +71,10 @@ public static void WriteUtf16(string path, string text) // Ensures an on-disk copy of `file` is UTF-16LE+BOM, writing a temp copy under // Paths.TempBundleContent\Encoding\\ only when a copy is actually needed. - // For tools that must be handed a file path (KDiff3.exe's command line) rather than - // raw text - an in-process engine that reads/writes strings directly doesn't need - // this at all, just ReadAnyEncoding/WriteUtf16 above. + // For a tool that must be handed a file path rather than raw text (the now-retired + // KDiff3 engine's command line was the original, and so far only, such caller - + // see docs/decisions/kdiff3-retirement.md) - an in-process engine that reads/writes + // strings directly doesn't need this at all, just ReadAnyEncoding/WriteUtf16 above. public static string EnsureUtf16File(FileInfo file, string role) { if (HasUtf16LeBom(file.FullName)) diff --git a/WitcherScriptMerger.Core/Tools/FileOpener.cs b/WitcherScriptMerger.Core/Tools/FileOpener.cs new file mode 100644 index 0000000..85511c2 --- /dev/null +++ b/WitcherScriptMerger.Core/Tools/FileOpener.cs @@ -0,0 +1,57 @@ +using System; +using System.Diagnostics; +using System.IO; + +namespace WitcherScriptMerger.Tools +{ + // "Open in the OS's default associated app" helper - Core-side (not the host + // project's Program.TryOpenFile) specifically so the GUI's interactive path and the + // CLI/MCP headless path can both reach it without Core referencing System.Windows.Forms. + // DiffPlexMergeEngine.MergeHeadless is the only real call site: when a genuine + // conflict writes a conflict-marker sidecar (see GetConflictMarkerPath), this is how + // it gets opened for the user to review - both the interactive path (Merge() just + // delegates to MergeHeadless()) and the headless CLI/MCP path funnel through that one + // call, so there's only ever one "open the sidecar" mechanism, not two. + // + // Deliberately not a call to Program.TryOpenFile even though that already exists and + // is used elsewhere (MergeReportForm's "Open Merged File" etc.): its non-.exe branch + // is a bare `Process.Start(path)` with no UseShellExecute=true, and on modern .NET + // (unlike .NET Framework, where UseShellExecute defaulted to true) that overload + // defaults UseShellExecute to false - meaning it tries to launch the target directly + // as a process image rather than through the shell's file association, throws + // Win32Exception for a plain text file, and that exception is silently swallowed by + // TryOpenFile's surrounding catch. That looks like a real pre-existing latent bug + // (out of scope to fix broadly here - smaller blast radius on an already high-stakes + // diff), but it means copying that exact pattern into this new code path would make + // the "opens it now for review" feature silently never actually open anything. Using + // UseShellExecute=true explicitly here avoids inheriting it. + public static class FileOpener + { + // A field, not a method call, so tests can substitute a fake and verify the exact + // path passed without a real process ever launching - the same + // swappable-static-dependency pattern AppState.Notifier already uses for + // testability elsewhere in this codebase. Defaults to the real implementation. + public static Func Open = TryOpen; + + public static bool TryOpen(string path) + { + if (!File.Exists(path)) + return false; + + try + { + Process.Start(new ProcessStartInfo(path) { UseShellExecute = true }); + return true; + } + catch + { + // Best-effort, same as the rest of this codebase's non-critical cleanup/UX + // helpers (e.g. DiffPlexMergeEngine.DeleteIfExists) - no file association, + // a denied launch, etc. shouldn't turn "needs manual resolution" into a + // harder failure than it already is. The conflict-marker sidecar itself is + // still on disk either way; only the convenience of auto-opening it fails. + return false; + } + } + } +} diff --git a/WitcherScriptMerger.Core/Tools/IMergeEngine.cs b/WitcherScriptMerger.Core/Tools/IMergeEngine.cs deleted file mode 100644 index b2eda71..0000000 --- a/WitcherScriptMerger.Core/Tools/IMergeEngine.cs +++ /dev/null @@ -1,49 +0,0 @@ -using System.IO; -using WitcherScriptMerger.Inventory; - -namespace WitcherScriptMerger.Tools -{ - public enum MergeEngineResult - { - AutoSolved, - NeedsManualResolution, - Failed, - } - - // Scaffolding introduced by the Core/host project split - NOT meant as a - // permanent pluggable-engine abstraction. It exists only so FileMerger (now in - // Core) can call a 3-way text merge without Core referencing Tools/KDiff3.cs's - // Win32 P/Invoke, which has to stay in the host project for now. The host - // project supplies the one real implementation (KDiff3MergeEngine) at startup - // via AppState.MergeEngine. A later unit that removes KDiff3 entirely will - // likely delete this interface and inline its replacement directly into - // FileMerger, unless a test project ends up depending on it as a seam. - public interface IMergeEngine - { - // Interactive: may open the merge tool's own UI and block until the user - // finishes or cancels. Returns AutoSolved on any successful save (whether - // auto-solved or manually resolved by the user), Failed on cancel/error. - // Never returns NeedsManualResolution - that's a headless-only concept. - MergeEngineResult Merge( - FileMerger.MergeSource source1, - FileMerger.MergeSource source2, - FileInfo vanillaFile, - string outputPath); - - // Headless: never blocks on user interaction. Detects an unresolved - // conflict itself and reports NeedsManualResolution instead of leaving a - // process hanging or a window open with nobody watching it. - MergeEngineResult MergeHeadless( - FileMerger.MergeSource source1, - FileMerger.MergeSource source2, - FileInfo vanillaFile, - string outputPath); - - // Whether the underlying merge tool's executable can actually be found. - // Exists so Paths.ValidateDependencyPaths() (Core) can validate the merge - // engine's dependency alongside QuickBMS/wcc_lite without Core referencing - // Tools/KDiff3.cs directly - that class stays in the host project for its - // Win32 P/Invoke, so Core can only reach it through this interface. - bool ValidateExePath(); - } -} diff --git a/WitcherScriptMerger.Tests/Tools/DiffPlexMergeEngineTests.cs b/WitcherScriptMerger.Tests/Tools/DiffPlexMergeEngineTests.cs index 252767f..9e45640 100644 --- a/WitcherScriptMerger.Tests/Tools/DiffPlexMergeEngineTests.cs +++ b/WitcherScriptMerger.Tests/Tools/DiffPlexMergeEngineTests.cs @@ -22,6 +22,13 @@ namespace WitcherScriptMerger.Tests.Tools // entire test run, not just fail one test. MergeSource's fields are all public, so // tests build it directly instead - this exercises DiffPlexMergeEngine exactly the // same way, since it only ever reads TextFile/Hash/Name off the struct. + // + // Any fixture that reaches a genuine-conflict outcome now also reaches + // FileOpener.Open (see MergeHeadless's comment) - those fixtures swap it for a stub + // in a try/finally around the real FileOpener.TryOpen, so no test run ever actually + // launches a process. All fixtures in this class run sequentially (xunit's default: + // one implicit collection per test class), so swapping this process-wide static field + // per-test is safe without extra locking. public class DiffPlexMergeEngineTests { [Fact] @@ -125,8 +132,14 @@ public void MergeHeadless_InterleavedIndependentEdits_SkipsWithoutWritingAnythin // full MergeHeadless path: since the "conflict marker" content itself would // have been built from the same untrustworthy piece indices, MergeHeadless // must not write a sidecar here either - this is the one case where - // DiffPlexMergeEngine can't even offer a conflict-marker starting point. + // DiffPlexMergeEngine can't even offer a conflict-marker starting point. No + // sidecar also means FileOpener.Open must never fire here - stubbed with a + // call counter (rather than the default real implementation) specifically to + // verify that negative, not just to avoid a real process launch. var dir = CreateTempDir(); + var previousOpener = FileOpener.Open; + var openCallCount = 0; + FileOpener.Open = _ => { ++openCallCount; return true; }; try { var vanillaPath = Path.Combine(dir, "vanilla.ws"); @@ -146,9 +159,11 @@ public void MergeHeadless_InterleavedIndependentEdits_SkipsWithoutWritingAnythin Assert.Equal(MergeEngineResult.NeedsManualResolution, result); Assert.False(File.Exists(outputPath)); Assert.False(File.Exists(DiffPlexMergeEngine.GetConflictMarkerPath(outputPath))); + Assert.Equal(0, openCallCount); } finally { + FileOpener.Open = previousOpener; Directory.Delete(dir, true); } } @@ -232,6 +247,12 @@ public void MergeHeadless_EncodingMismatch_NormalizesAndProducesUtf16LEWithBomOu public void MergeHeadless_GenuineConflict_WritesSidecarMarkerFileNotOutputPath() { var dir = CreateTempDir(); + var previousOpener = FileOpener.Open; + // A genuine conflict now also opens the sidecar via FileOpener (see + // MergeHeadless_GenuineConflict_OpensSidecarViaFileOpener below) - stubbed out + // here so this fixture, which only cares about the sidecar file itself, never + // launches a real process during a test run. + FileOpener.Open = _ => true; try { var vanillaPath = Path.Combine(dir, "vanilla.ws"); @@ -267,6 +288,98 @@ public void MergeHeadless_GenuineConflict_WritesSidecarMarkerFileNotOutputPath() } finally { + FileOpener.Open = previousOpener; + Directory.Delete(dir, true); + } + } + + [Fact] + public void MergeHeadless_GenuineConflict_OpensSidecarViaFileOpener() + { + // The headline new-user-experience change this unit adds: a genuine conflict's + // sidecar isn't just written to disk, it's opened for the user via FileOpener - + // the same call fires whether this was reached via the CLI/MCP headless path + // directly or via the GUI's interactive Merge() (which just delegates to this + // method). FileOpener.Open is swapped for a recording stub rather than letting + // the real implementation run, so this test verifies the call - the exact path + // passed - without actually launching a process during a test run. + var dir = CreateTempDir(); + var previousOpener = FileOpener.Open; + string openedPath = null; + var openCallCount = 0; + FileOpener.Open = path => + { + openedPath = path; + ++openCallCount; + return true; + }; + try + { + var vanillaPath = Path.Combine(dir, "vanilla.ws"); + FileEncoding.WriteUtf16(vanillaPath, "x = 1;\r\n"); + var mod1Path = Path.Combine(dir, "mod1.ws"); + FileEncoding.WriteUtf16(mod1Path, "x = 2;\r\n"); + var mod2Path = Path.Combine(dir, "mod2.ws"); + FileEncoding.WriteUtf16(mod2Path, "x = 3;\r\n"); + + var outputPath = Path.Combine(dir, "merged.ws"); + + var source1 = new FileMerger.MergeSource { TextFile = new FileInfo(mod1Path), Name = "modA" }; + var source2 = new FileMerger.MergeSource { TextFile = new FileInfo(mod2Path), Name = "modB" }; + + var result = new DiffPlexMergeEngine().MergeHeadless(source1, source2, new FileInfo(vanillaPath), outputPath); + + Assert.Equal(MergeEngineResult.NeedsManualResolution, result); + Assert.Equal(1, openCallCount); + Assert.Equal(DiffPlexMergeEngine.GetConflictMarkerPath(outputPath), openedPath); + } + finally + { + FileOpener.Open = previousOpener; + Directory.Delete(dir, true); + } + } + + [Fact] + public void MergeHeadless_OpenConflictMarkersFalse_WritesSidecarButNeverCallsFileOpener() + { + // Regression test for a real bug caught in code review before this shipped: + // FileMerger.MergeTextHeadless passes openConflictMarkers: !dryRun, so a dry + // run (MergeConflictsHeadless(dryRun: true), including the MCP merge_conflicts + // tool's dryRun option) must never launch a real editor/process for a genuine + // conflict - that's exactly the kind of surprise side effect a "preview only" + // operation promises not to have. The sidecar file itself is still written + // (pre-existing behavior, unchanged by this parameter) so a dry run's summary + // can still point at well-formed conflict-marker content if a caller wants to + // inspect it - only the auto-open is suppressed. + var dir = CreateTempDir(); + var previousOpener = FileOpener.Open; + var openCallCount = 0; + FileOpener.Open = _ => { ++openCallCount; return true; }; + try + { + var vanillaPath = Path.Combine(dir, "vanilla.ws"); + FileEncoding.WriteUtf16(vanillaPath, "x = 1;\r\n"); + var mod1Path = Path.Combine(dir, "mod1.ws"); + FileEncoding.WriteUtf16(mod1Path, "x = 2;\r\n"); + var mod2Path = Path.Combine(dir, "mod2.ws"); + FileEncoding.WriteUtf16(mod2Path, "x = 3;\r\n"); + + var outputPath = Path.Combine(dir, "merged.ws"); + + var source1 = new FileMerger.MergeSource { TextFile = new FileInfo(mod1Path), Name = "modA" }; + var source2 = new FileMerger.MergeSource { TextFile = new FileInfo(mod2Path), Name = "modB" }; + + var result = new DiffPlexMergeEngine().MergeHeadless( + source1, source2, new FileInfo(vanillaPath), outputPath, openConflictMarkers: false); + + Assert.Equal(MergeEngineResult.NeedsManualResolution, result); + Assert.True(File.Exists(DiffPlexMergeEngine.GetConflictMarkerPath(outputPath))); + Assert.Equal(0, openCallCount); + } + finally + { + FileOpener.Open = previousOpener; Directory.Delete(dir, true); } } @@ -280,6 +393,8 @@ public void MergeHeadless_RetryAfterConflictThatNowAutoSolves_RemovesStaleSideca // to the fresh output indefinitely - MergeHeadless deletes it on the // AutoSolved path specifically to avoid that. var dir = CreateTempDir(); + var previousOpener = FileOpener.Open; + FileOpener.Open = _ => true; // see the fixture above for why this is stubbed try { var vanillaPath = Path.Combine(dir, "vanilla.ws"); @@ -309,6 +424,7 @@ public void MergeHeadless_RetryAfterConflictThatNowAutoSolves_RemovesStaleSideca } finally { + FileOpener.Open = previousOpener; Directory.Delete(dir, true); } } @@ -350,11 +466,15 @@ public void MergeHeadless_NoVanillaFile_SkipsWithoutWritingAnything() [Fact] public void Merge_Interactive_NeverReturnsNeedsManualResolution() { - // IMergeEngine.Merge's contract explicitly forbids ever returning - // NeedsManualResolution (that's a headless-only concept) - DiffPlexMergeEngine - // has no UI to resolve a conflict interactively, so a genuine conflict must - // come back as Failed instead. + // Merge's contract explicitly forbids ever returning NeedsManualResolution + // (that's a headless-only concept) - DiffPlexMergeEngine has no UI to resolve + // a conflict interactively, so a genuine conflict must come back as Failed + // instead. Merge() delegates straight to MergeHeadless(), which also means this + // genuine conflict writes a sidecar and calls FileOpener.Open on the interactive + // path too - stubbed here for the same reason as the headless fixtures above. var dir = CreateTempDir(); + var previousOpener = FileOpener.Open; + FileOpener.Open = _ => true; try { var vanillaPath = Path.Combine(dir, "vanilla.ws"); @@ -375,6 +495,7 @@ public void Merge_Interactive_NeverReturnsNeedsManualResolution() } finally { + FileOpener.Open = previousOpener; Directory.Delete(dir, true); } } diff --git a/WitcherScriptMerger.Tests/Tools/KDiff3CrossCheckTests.cs b/WitcherScriptMerger.Tests/Tools/KDiff3CrossCheckTests.cs index 1d8eac8..834f9b2 100644 --- a/WitcherScriptMerger.Tests/Tools/KDiff3CrossCheckTests.cs +++ b/WitcherScriptMerger.Tests/Tools/KDiff3CrossCheckTests.cs @@ -12,22 +12,34 @@ namespace WitcherScriptMerger.Tests.Tools // CONTRIBUTING.md, a committed test must not require or hardcode a machine-specific // path. // + // KDiff3 was retired as WSM's shipped text-merge engine/dependency - see + // docs/decisions/kdiff3-retirement.md. This class doesn't exercise anything WSM itself + // still does: it's a standalone oracle, invoking a real kdiff3.exe binary you happen to + // have locally (WSM no longer bundles or requires one) purely to cross-check + // DiffPlexMergeEngine's output against a mature, independent 3-way merge + // implementation - kept deliberately, not by oversight, since DiffPlexMergeEngine is + // now the sole engine and has a known upstream bug (see CLAUDE.md's Compatibility + // constraints) that this cross-check has no bearing on but that makes independent + // verification of the auto-solvable cases it does cover worth keeping. + // // Deliberately narrow in scope: only auto-solvable scenarios are compared here // (whitespace-only, and non-overlapping edits). A genuine two-sided conflict is NOT // cross-checked against the real binary from this test project, because safely // automating KDiff3 headlessly for that case needs the window-persistence detection - // documented in CLAUDE.md's compatibility constraints (a ~250ms poll interval that's + // documented in docs/decisions/kdiff3-retirement.md (a ~250ms poll interval that's // itself load-bearing, and a window that can't be hidden without hanging the merge - // entirely) - that logic (Win32 P/Invoke) lives in the host project's Tools/KDiff3.cs, - // which this Core-only test project intentionally doesn't reference. Below uses a - // single bounded Process.WaitForExit with a kill-on-timeout fallback instead, safe - // only because both scenarios here are designed to be cleanly auto-solvable - per - // CLAUDE.md, an untouched, auto-solvable KDiff3 launch reliably exits in a few - // seconds regardless of file size. + // entirely) - that logic (Win32 P/Invoke) lived in the host project's since-deleted + // Tools/KDiff3.cs, which this Core-only test project never referenced anyway. Below + // uses a single bounded Process.WaitForExit with a kill-on-timeout fallback instead, + // safe only because both scenarios here are designed to be cleanly auto-solvable - per + // docs/decisions/kdiff3-retirement.md, an untouched, auto-solvable KDiff3 launch + // reliably exits in a few seconds regardless of file size. // // Running this locally (WSM_TEST_GAME_DIR set) will briefly show KDiff3's window and - // steal foreground focus, twice - the same documented behavior CLAUDE.md describes - // for the real headless CLI path. That's expected, not a bug in this test. + // steal foreground focus, twice - the same documented behavior + // docs/decisions/kdiff3-retirement.md describes for the old headless CLI path this + // class's launch mechanism deliberately mirrors. That's expected, not a bug in this + // test. public class KDiff3CrossCheckTests { [Fact] diff --git a/WitcherScriptMerger/App.config b/WitcherScriptMerger/App.config index 35a78ea..d66e85d 100644 --- a/WitcherScriptMerger/App.config +++ b/WitcherScriptMerger/App.config @@ -16,23 +16,14 @@ CollapseNotMergeable Whether to auto-collapse conflicts that can't be merged ValidateMergeSources Whether to prompt to delete merges outdated by changes in mod files ValidateCustomLoadOrder Whether to detect mods.settings file on refresh & make sure it's configured to load merged files first -ReviewEachMerge Whether to show the text comparison UI for every merge, instead of just ones that aren't auto-solvable -ShowPathsInKDiff3 Whether to show file paths in KDiff3 instead of just Vanilla, modName1, modName2 PlayCompletionSounds Whether to play a sound after merging or packing a bundle ReportAfterMerge Whether to show a report after merging 2 files, with buttons to open files/directories ReportAfterPack Whether to show a report after packing a bundle, with list of contents & button to open directory MergedModName Which mod folder to save merges in (should be 1st alphabetically, so the game loads it before others) -KDiff3Path Where KDiff3.exe is located QuickBmsPath Where quickbms.exe is located QuickBmsPluginPath Where the witcher3.bms plugin for QuickBMS is located WccLitePath Where wcc_lite.exe is located - -MergeEngine Which text-merge engine to use: "kdiff3" (default) or "diffplex". - DiffPlex is an in-process alternative to KDiff3 that needs no - external binary - see CLAUDE.md's Interactive vs. headless split - section. Not the default yet; still being verified against KDiff3 - on real conflicts. --> @@ -48,17 +39,13 @@ MergeEngine Which text-merge engine to use: "kdiff3" (default) or "d - - - - diff --git a/WitcherScriptMerger/Forms/DependencyForm.Designer.cs b/WitcherScriptMerger/Forms/DependencyForm.Designer.cs index 6c6f421..05be8e4 100644 --- a/WitcherScriptMerger/Forms/DependencyForm.Designer.cs +++ b/WitcherScriptMerger/Forms/DependencyForm.Designer.cs @@ -29,12 +29,6 @@ protected override void Dispose(bool disposing) private void InitializeComponent() { System.ComponentModel.ComponentResourceManager resources = new System.ComponentModel.ComponentResourceManager(typeof(DependencyForm)); - this.grpKDiff3 = new System.Windows.Forms.GroupBox(); - this.lblKDiff3Msg = new System.Windows.Forms.Label(); - this.lblKDiff3Path = new System.Windows.Forms.Label(); - this.btnKDiff3Path = new System.Windows.Forms.Button(); - this.txtKDiff3Path = new System.Windows.Forms.TextBox(); - this.lnkKDiff3 = new System.Windows.Forms.LinkLabel(); this.lblPrompt = new System.Windows.Forms.Label(); this.grpBms = new System.Windows.Forms.GroupBox(); this.lblBmsMsg = new System.Windows.Forms.Label(); @@ -56,86 +50,11 @@ private void InitializeComponent() this.btnOK = new System.Windows.Forms.Button(); this.btnCancel = new System.Windows.Forms.Button(); this.lnkBms = new System.Windows.Forms.LinkLabel(); - this.grpKDiff3.SuspendLayout(); this.grpBms.SuspendLayout(); this.grpBmsPlugin.SuspendLayout(); this.grpWccLite.SuspendLayout(); this.SuspendLayout(); - // - // grpKDiff3 - // - this.grpKDiff3.Anchor = ((System.Windows.Forms.AnchorStyles)(((System.Windows.Forms.AnchorStyles.Top | System.Windows.Forms.AnchorStyles.Left) - | System.Windows.Forms.AnchorStyles.Right))); - this.grpKDiff3.Controls.Add(this.lblKDiff3Msg); - this.grpKDiff3.Controls.Add(this.lblKDiff3Path); - this.grpKDiff3.Controls.Add(this.btnKDiff3Path); - this.grpKDiff3.Controls.Add(this.txtKDiff3Path); - this.grpKDiff3.Font = new System.Drawing.Font("Microsoft Sans Serif", 8.25F, System.Drawing.FontStyle.Bold, System.Drawing.GraphicsUnit.Point, ((byte)(0))); - this.grpKDiff3.Location = new System.Drawing.Point(9, 60); - this.grpKDiff3.Name = "grpKDiff3"; - this.grpKDiff3.Size = new System.Drawing.Size(445, 80); - this.grpKDiff3.TabIndex = 1; - this.grpKDiff3.TabStop = false; - this.grpKDiff3.Text = "KDiff3.exe"; - // - // lblKDiff3Msg - // - this.lblKDiff3Msg.AutoSize = true; - this.lblKDiff3Msg.Font = new System.Drawing.Font("Microsoft Sans Serif", 8.25F, System.Drawing.FontStyle.Regular, System.Drawing.GraphicsUnit.Point, ((byte)(0))); - this.lblKDiff3Msg.Location = new System.Drawing.Point(7, 52); - this.lblKDiff3Msg.Name = "lblKDiff3Msg"; - this.lblKDiff3Msg.Size = new System.Drawing.Size(399, 13); - this.lblKDiff3Msg.TabIndex = 4; - this.lblKDiff3Msg.Text = "Script Merger uses this open-source tool by Joachim Eibl to create merged text fi" + - "les."; - // - // lblKDiff3Path - // - this.lblKDiff3Path.AutoSize = true; - this.lblKDiff3Path.Font = new System.Drawing.Font("Microsoft Sans Serif", 8.25F, System.Drawing.FontStyle.Regular, System.Drawing.GraphicsUnit.Point, ((byte)(0))); - this.lblKDiff3Path.Location = new System.Drawing.Point(7, 27); - this.lblKDiff3Path.Name = "lblKDiff3Path"; - this.lblKDiff3Path.Size = new System.Drawing.Size(32, 13); - this.lblKDiff3Path.TabIndex = 3; - this.lblKDiff3Path.Text = "Path:"; - // - // btnKDiff3Path - // - this.btnKDiff3Path.Anchor = ((System.Windows.Forms.AnchorStyles)((System.Windows.Forms.AnchorStyles.Top | System.Windows.Forms.AnchorStyles.Right))); - this.btnKDiff3Path.Font = new System.Drawing.Font("Microsoft Sans Serif", 8.25F, System.Drawing.FontStyle.Regular, System.Drawing.GraphicsUnit.Point, ((byte)(0))); - this.btnKDiff3Path.Location = new System.Drawing.Point(413, 22); - this.btnKDiff3Path.Name = "btnKDiff3Path"; - this.btnKDiff3Path.Size = new System.Drawing.Size(26, 23); - this.btnKDiff3Path.TabIndex = 1; - this.btnKDiff3Path.Text = "..."; - this.btnKDiff3Path.UseVisualStyleBackColor = true; - this.btnKDiff3Path.Click += new System.EventHandler(this.btnKDiff3Path_Click); - // - // txtKDiff3Path - // - this.txtKDiff3Path.Anchor = ((System.Windows.Forms.AnchorStyles)(((System.Windows.Forms.AnchorStyles.Top | System.Windows.Forms.AnchorStyles.Left) - | System.Windows.Forms.AnchorStyles.Right))); - this.txtKDiff3Path.Font = new System.Drawing.Font("Microsoft Sans Serif", 8.25F, System.Drawing.FontStyle.Regular, System.Drawing.GraphicsUnit.Point, ((byte)(0))); - this.txtKDiff3Path.Location = new System.Drawing.Point(45, 24); - this.txtKDiff3Path.Name = "txtKDiff3Path"; - this.txtKDiff3Path.Size = new System.Drawing.Size(362, 20); - this.txtKDiff3Path.TabIndex = 0; - this.txtKDiff3Path.TextChanged += new System.EventHandler(this.exe_TextChanged); - // - // lnkKDiff3 - // - this.lnkKDiff3.Anchor = ((System.Windows.Forms.AnchorStyles)((System.Windows.Forms.AnchorStyles.Top | System.Windows.Forms.AnchorStyles.Right))); - this.lnkKDiff3.AutoSize = true; - this.lnkKDiff3.LinkArea = new System.Windows.Forms.LinkArea(14, 53); - this.lnkKDiff3.Location = new System.Drawing.Point(311, 60); - this.lnkKDiff3.Name = "lnkKDiff3"; - this.lnkKDiff3.Size = new System.Drawing.Size(137, 17); - this.lnkKDiff3.TabIndex = 0; - this.lnkKDiff3.TabStop = true; - this.lnkKDiff3.Text = "Download from KDiff3 Site"; - this.lnkKDiff3.UseCompatibleTextRendering = true; - this.lnkKDiff3.LinkClicked += new System.Windows.Forms.LinkLabelLinkClickedEventHandler(this.lnkKDiff3_LinkClicked); - // + // // lblPrompt // this.lblPrompt.AutoSize = true; @@ -156,7 +75,7 @@ private void InitializeComponent() this.grpBms.Controls.Add(this.btnBmsPath); this.grpBms.Controls.Add(this.txtBmsPath); this.grpBms.Font = new System.Drawing.Font("Microsoft Sans Serif", 8.25F, System.Drawing.FontStyle.Bold, System.Drawing.GraphicsUnit.Point, ((byte)(0))); - this.grpBms.Location = new System.Drawing.Point(9, 156); + this.grpBms.Location = new System.Drawing.Point(9, 60); this.grpBms.Name = "grpBms"; this.grpBms.Size = new System.Drawing.Size(445, 80); this.grpBms.TabIndex = 4; @@ -212,7 +131,7 @@ private void InitializeComponent() this.lnkBmsPlugin.Anchor = ((System.Windows.Forms.AnchorStyles)((System.Windows.Forms.AnchorStyles.Top | System.Windows.Forms.AnchorStyles.Right))); this.lnkBmsPlugin.AutoSize = true; this.lnkBmsPlugin.LinkArea = new System.Windows.Forms.LinkArea(14, 53); - this.lnkBmsPlugin.Location = new System.Drawing.Point(292, 252); + this.lnkBmsPlugin.Location = new System.Drawing.Point(292, 156); this.lnkBmsPlugin.Name = "lnkBmsPlugin"; this.lnkBmsPlugin.Size = new System.Drawing.Size(159, 17); this.lnkBmsPlugin.TabIndex = 5; @@ -230,7 +149,7 @@ private void InitializeComponent() this.grpBmsPlugin.Controls.Add(this.btnBmsPluginPath); this.grpBmsPlugin.Controls.Add(this.txtBmsPluginPath); this.grpBmsPlugin.Font = new System.Drawing.Font("Microsoft Sans Serif", 8.25F, System.Drawing.FontStyle.Bold, System.Drawing.GraphicsUnit.Point, ((byte)(0))); - this.grpBmsPlugin.Location = new System.Drawing.Point(12, 252); + this.grpBmsPlugin.Location = new System.Drawing.Point(12, 156); this.grpBmsPlugin.Name = "grpBmsPlugin"; this.grpBmsPlugin.Size = new System.Drawing.Size(445, 80); this.grpBmsPlugin.TabIndex = 6; @@ -285,7 +204,7 @@ private void InitializeComponent() this.lnkWccLite.Anchor = ((System.Windows.Forms.AnchorStyles)((System.Windows.Forms.AnchorStyles.Top | System.Windows.Forms.AnchorStyles.Right))); this.lnkWccLite.AutoSize = true; this.lnkWccLite.LinkArea = new System.Windows.Forms.LinkArea(14, 53); - this.lnkWccLite.Location = new System.Drawing.Point(305, 348); + this.lnkWccLite.Location = new System.Drawing.Point(305, 252); this.lnkWccLite.Name = "lnkWccLite"; this.lnkWccLite.Size = new System.Drawing.Size(146, 17); this.lnkWccLite.TabIndex = 7; @@ -303,7 +222,7 @@ private void InitializeComponent() this.grpWccLite.Controls.Add(this.btnWccLitePath); this.grpWccLite.Controls.Add(this.txtWccLitePath); this.grpWccLite.Font = new System.Drawing.Font("Microsoft Sans Serif", 8.25F, System.Drawing.FontStyle.Bold, System.Drawing.GraphicsUnit.Point, ((byte)(0))); - this.grpWccLite.Location = new System.Drawing.Point(12, 348); + this.grpWccLite.Location = new System.Drawing.Point(12, 252); this.grpWccLite.Name = "grpWccLite"; this.grpWccLite.Size = new System.Drawing.Size(445, 80); this.grpWccLite.TabIndex = 8; @@ -358,7 +277,7 @@ private void InitializeComponent() // this.btnOK.Anchor = ((System.Windows.Forms.AnchorStyles)((System.Windows.Forms.AnchorStyles.Top | System.Windows.Forms.AnchorStyles.Right))); this.btnOK.DialogResult = System.Windows.Forms.DialogResult.Cancel; - this.btnOK.Location = new System.Drawing.Point(251, 442); + this.btnOK.Location = new System.Drawing.Point(251, 346); this.btnOK.Name = "btnOK"; this.btnOK.Size = new System.Drawing.Size(100, 23); this.btnOK.TabIndex = 9; @@ -370,7 +289,7 @@ private void InitializeComponent() // this.btnCancel.Anchor = ((System.Windows.Forms.AnchorStyles)((System.Windows.Forms.AnchorStyles.Top | System.Windows.Forms.AnchorStyles.Right))); this.btnCancel.DialogResult = System.Windows.Forms.DialogResult.Cancel; - this.btnCancel.Location = new System.Drawing.Point(357, 442); + this.btnCancel.Location = new System.Drawing.Point(357, 346); this.btnCancel.Name = "btnCancel"; this.btnCancel.Size = new System.Drawing.Size(100, 23); this.btnCancel.TabIndex = 10; @@ -383,7 +302,7 @@ private void InitializeComponent() this.lnkBms.Anchor = ((System.Windows.Forms.AnchorStyles)((System.Windows.Forms.AnchorStyles.Top | System.Windows.Forms.AnchorStyles.Right))); this.lnkBms.AutoSize = true; this.lnkBms.LinkArea = new System.Windows.Forms.LinkArea(14, 53); - this.lnkBms.Location = new System.Drawing.Point(289, 156); + this.lnkBms.Location = new System.Drawing.Point(289, 60); this.lnkBms.Name = "lnkBms"; this.lnkBms.Size = new System.Drawing.Size(159, 17); this.lnkBms.TabIndex = 2; @@ -398,7 +317,7 @@ private void InitializeComponent() this.AutoScaleDimensions = new System.Drawing.SizeF(6F, 13F); this.AutoScaleMode = System.Windows.Forms.AutoScaleMode.Font; this.CancelButton = this.btnOK; - this.ClientSize = new System.Drawing.Size(469, 477); + this.ClientSize = new System.Drawing.Size(469, 381); this.ControlBox = false; this.Controls.Add(this.lnkBms); this.Controls.Add(this.btnCancel); @@ -409,17 +328,13 @@ private void InitializeComponent() this.Controls.Add(this.grpBmsPlugin); this.Controls.Add(this.lblPrompt); this.Controls.Add(this.grpBms); - this.Controls.Add(this.lnkKDiff3); - this.Controls.Add(this.grpKDiff3); this.Icon = ((System.Drawing.Icon)(resources.GetObject("$this.Icon"))); - this.MaximumSize = new System.Drawing.Size(1920, 515); - this.MinimumSize = new System.Drawing.Size(485, 515); + this.MaximumSize = new System.Drawing.Size(1920, 419); + this.MinimumSize = new System.Drawing.Size(485, 419); this.Name = "DependencyForm"; this.StartPosition = System.Windows.Forms.FormStartPosition.CenterScreen; this.Text = "Dependency Locations"; this.Load += new System.EventHandler(this.DependencyForm_Load); - this.grpKDiff3.ResumeLayout(false); - this.grpKDiff3.PerformLayout(); this.grpBms.ResumeLayout(false); this.grpBms.PerformLayout(); this.grpBmsPlugin.ResumeLayout(false); @@ -432,12 +347,6 @@ private void InitializeComponent() } #endregion - private System.Windows.Forms.GroupBox grpKDiff3; - private System.Windows.Forms.TextBox txtKDiff3Path; - private System.Windows.Forms.Label lblKDiff3Msg; - private System.Windows.Forms.Label lblKDiff3Path; - private System.Windows.Forms.Button btnKDiff3Path; - private System.Windows.Forms.LinkLabel lnkKDiff3; private System.Windows.Forms.Label lblPrompt; private System.Windows.Forms.GroupBox grpBms; private System.Windows.Forms.Label lblBmsMsg; diff --git a/WitcherScriptMerger/Forms/DependencyForm.cs b/WitcherScriptMerger/Forms/DependencyForm.cs index ebef874..5ff41cb 100644 --- a/WitcherScriptMerger/Forms/DependencyForm.cs +++ b/WitcherScriptMerger/Forms/DependencyForm.cs @@ -13,8 +13,7 @@ bool AreAnyPathsChanged { get { - return (!txtKDiff3Path.Text.EqualsIgnoreCase(KDiff3.ExePath) || - !txtBmsPath.Text.EqualsIgnoreCase(QuickBms.ExePath) || + return (!txtBmsPath.Text.EqualsIgnoreCase(QuickBms.ExePath) || !txtBmsPluginPath.Text.EqualsIgnoreCase(QuickBms.PluginPath) || !txtWccLitePath.Text.EqualsIgnoreCase(WccLite.ExePath)); } @@ -27,7 +26,6 @@ public DependencyForm() void DependencyForm_Load(object sender, EventArgs e) { - txtKDiff3Path.Text = KDiff3.ExePath; txtBmsPath.Text = QuickBms.ExePath; txtBmsPluginPath.Text = QuickBms.PluginPath; txtWccLitePath.Text = WccLite.ExePath; @@ -37,7 +35,6 @@ void DependencyForm_Load(object sender, EventArgs e) void btnOK_Click(object sender, EventArgs e) { var allValid = - Color.LightGreen == txtKDiff3Path.BackColor && Color.LightGreen == txtBmsPath.BackColor && Color.LightGreen == txtBmsPluginPath.BackColor && Color.LightGreen == txtWccLitePath.BackColor; @@ -55,7 +52,6 @@ void btnOK_Click(object sender, EventArgs e) if (AreAnyPathsChanged) { - KDiff3.ExePath = UpdatePathSetting(KDiff3.ExePath, txtKDiff3Path.Text, "Kdiff3Path"); QuickBms.ExePath = UpdatePathSetting(QuickBms.ExePath, txtBmsPath.Text, "QuickBmsPath"); QuickBms.PluginPath = UpdatePathSetting(QuickBms.PluginPath, txtBmsPluginPath.Text, "QuickBmsPluginPath"); WccLite.ExePath = UpdatePathSetting(WccLite.ExePath, txtWccLitePath.Text, "WccLitePath"); @@ -82,11 +78,6 @@ void btnCancel_Click(object sender, EventArgs e) #region Selecting Files - void btnKDiff3Path_Click(object sender, EventArgs e) - { - GetUserFileChoice(txtKDiff3Path, "Executables|*.exe"); - } - void btnBmsPath_Click(object sender, EventArgs e) { GetUserFileChoice(txtBmsPath, "Executables|*.exe"); @@ -116,11 +107,6 @@ void GetUserFileChoice(TextBox txt, string filter) #region Clicking Links - void lnkKDiff3_LinkClicked(object sender, LinkLabelLinkClickedEventArgs e) - { - Process.Start("http://kdiff3.sourceforge.net/"); - } - void lnkBms_LinkClicked(object sender, LinkLabelLinkClickedEventArgs e) { Process.Start("http://aluigi.altervista.org/quickbms.htm"); diff --git a/WitcherScriptMerger/Forms/OptionsForm.Designer.cs b/WitcherScriptMerger/Forms/OptionsForm.Designer.cs index 9283a4d..22c434c 100644 --- a/WitcherScriptMerger/Forms/OptionsForm.Designer.cs +++ b/WitcherScriptMerger/Forms/OptionsForm.Designer.cs @@ -42,9 +42,7 @@ private void InitializeComponent() this.grpMerging = new System.Windows.Forms.GroupBox(); this.chkPackReport = new System.Windows.Forms.CheckBox(); this.chkMergeReport = new System.Windows.Forms.CheckBox(); - this.chkShowPathsInKDiff3 = new System.Windows.Forms.CheckBox(); this.chkCompletionSounds = new System.Windows.Forms.CheckBox(); - this.chkReviewEachMerge = new System.Windows.Forms.CheckBox(); this.grpAutocollapse = new System.Windows.Forms.GroupBox(); this.chkCollapseCustomLoadOrder = new System.Windows.Forms.CheckBox(); this.chkCollapseNotMergeable = new System.Windows.Forms.CheckBox(); @@ -176,72 +174,48 @@ private void InitializeComponent() // this.grpMerging.Controls.Add(this.chkPackReport); this.grpMerging.Controls.Add(this.chkMergeReport); - this.grpMerging.Controls.Add(this.chkShowPathsInKDiff3); this.grpMerging.Controls.Add(this.chkCompletionSounds); - this.grpMerging.Controls.Add(this.chkReviewEachMerge); this.grpMerging.Font = new System.Drawing.Font("Microsoft Sans Serif", 8.25F, System.Drawing.FontStyle.Bold, System.Drawing.GraphicsUnit.Point, ((byte)(0))); this.grpMerging.Location = new System.Drawing.Point(12, 291); this.grpMerging.Name = "grpMerging"; - this.grpMerging.Size = new System.Drawing.Size(282, 140); + this.grpMerging.Size = new System.Drawing.Size(282, 94); this.grpMerging.TabIndex = 1; this.grpMerging.TabStop = false; this.grpMerging.Text = "Merging"; - // + // // chkPackReport - // + // this.chkPackReport.AutoSize = true; this.chkPackReport.Font = new System.Drawing.Font("Microsoft Sans Serif", 8.25F, System.Drawing.FontStyle.Regular, System.Drawing.GraphicsUnit.Point, ((byte)(0))); - this.chkPackReport.Location = new System.Drawing.Point(6, 111); + this.chkPackReport.Location = new System.Drawing.Point(6, 65); this.chkPackReport.Name = "chkPackReport"; this.chkPackReport.Size = new System.Drawing.Size(183, 17); - this.chkPackReport.TabIndex = 7; + this.chkPackReport.TabIndex = 5; this.chkPackReport.Text = "Show report after packing bundle"; this.chkPackReport.UseVisualStyleBackColor = true; - // + // // chkMergeReport - // + // this.chkMergeReport.AutoSize = true; this.chkMergeReport.Font = new System.Drawing.Font("Microsoft Sans Serif", 8.25F, System.Drawing.FontStyle.Regular, System.Drawing.GraphicsUnit.Point, ((byte)(0))); - this.chkMergeReport.Location = new System.Drawing.Point(6, 88); + this.chkMergeReport.Location = new System.Drawing.Point(6, 42); this.chkMergeReport.Name = "chkMergeReport"; this.chkMergeReport.Size = new System.Drawing.Size(166, 17); - this.chkMergeReport.TabIndex = 6; + this.chkMergeReport.TabIndex = 4; this.chkMergeReport.Text = "Show report after each merge"; this.chkMergeReport.UseVisualStyleBackColor = true; - // - // chkShowPathsInKDiff3 - // - this.chkShowPathsInKDiff3.AutoSize = true; - this.chkShowPathsInKDiff3.Font = new System.Drawing.Font("Microsoft Sans Serif", 8.25F, System.Drawing.FontStyle.Regular, System.Drawing.GraphicsUnit.Point, ((byte)(0))); - this.chkShowPathsInKDiff3.Location = new System.Drawing.Point(6, 42); - this.chkShowPathsInKDiff3.Name = "chkShowPathsInKDiff3"; - this.chkShowPathsInKDiff3.Size = new System.Drawing.Size(141, 17); - this.chkShowPathsInKDiff3.TabIndex = 5; - this.chkShowPathsInKDiff3.Text = "Show file paths in KDiff3"; - this.chkShowPathsInKDiff3.UseVisualStyleBackColor = true; - // + // // chkCompletionSounds - // + // this.chkCompletionSounds.AutoSize = true; this.chkCompletionSounds.Font = new System.Drawing.Font("Microsoft Sans Serif", 8.25F, System.Drawing.FontStyle.Regular, System.Drawing.GraphicsUnit.Point, ((byte)(0))); - this.chkCompletionSounds.Location = new System.Drawing.Point(6, 65); + this.chkCompletionSounds.Location = new System.Drawing.Point(6, 19); this.chkCompletionSounds.Name = "chkCompletionSounds"; this.chkCompletionSounds.Size = new System.Drawing.Size(137, 17); - this.chkCompletionSounds.TabIndex = 4; + this.chkCompletionSounds.TabIndex = 3; this.chkCompletionSounds.Text = "Play completion sounds"; this.chkCompletionSounds.UseVisualStyleBackColor = true; - // - // chkReviewEachMerge - // - this.chkReviewEachMerge.AutoSize = true; - this.chkReviewEachMerge.Font = new System.Drawing.Font("Microsoft Sans Serif", 8.25F, System.Drawing.FontStyle.Regular, System.Drawing.GraphicsUnit.Point, ((byte)(0))); - this.chkReviewEachMerge.Location = new System.Drawing.Point(6, 19); - this.chkReviewEachMerge.Name = "chkReviewEachMerge"; - this.chkReviewEachMerge.Size = new System.Drawing.Size(271, 17); - this.chkReviewEachMerge.TabIndex = 3; - this.chkReviewEachMerge.Text = "Review each merge in KDiff3 (even if auto-solvable)"; - this.chkReviewEachMerge.UseVisualStyleBackColor = true; - // + // // grpAutocollapse // this.grpAutocollapse.Controls.Add(this.chkCollapseCustomLoadOrder); @@ -359,9 +333,7 @@ private void InitializeComponent() private System.Windows.Forms.GroupBox grpMerging; private System.Windows.Forms.CheckBox chkPackReport; private System.Windows.Forms.CheckBox chkMergeReport; - private System.Windows.Forms.CheckBox chkShowPathsInKDiff3; private System.Windows.Forms.CheckBox chkCompletionSounds; - private System.Windows.Forms.CheckBox chkReviewEachMerge; private System.Windows.Forms.GroupBox grpAutocollapse; private System.Windows.Forms.CheckBox chkCollapseCustomLoadOrder; private System.Windows.Forms.CheckBox chkCollapseNotMergeable; diff --git a/WitcherScriptMerger/Forms/OptionsForm.cs b/WitcherScriptMerger/Forms/OptionsForm.cs index 2e8583c..415e547 100644 --- a/WitcherScriptMerger/Forms/OptionsForm.cs +++ b/WitcherScriptMerger/Forms/OptionsForm.cs @@ -22,8 +22,6 @@ void Options_Load(object sender, EventArgs e) chkPromptOutdatedMerge.Checked = Program.Settings.Get("ValidateMergeSources"); chkPromptPrioritize.Checked = Program.Settings.Get("ValidateCustomLoadOrder"); - chkReviewEachMerge.Checked = Program.Settings.Get("ReviewEachMerge"); - chkShowPathsInKDiff3.Checked = Program.Settings.Get("ShowPathsInKDiff3"); chkCompletionSounds.Checked = Program.Settings.Get("PlayCompletionSounds"); chkMergeReport.Checked = Program.Settings.Get("ReportAfterMerge"); chkPackReport.Checked = Program.Settings.Get("ReportAfterPack"); @@ -62,8 +60,6 @@ void Save() Program.Settings.Set("ValidateMergeSources", chkPromptOutdatedMerge.Checked); Program.Settings.Set("ValidateCustomLoadOrder", chkPromptPrioritize.Checked); - Program.Settings.Set("ReviewEachMerge", chkReviewEachMerge.Checked); - Program.Settings.Set("ShowPathsInKDiff3", chkShowPathsInKDiff3.Checked); Program.Settings.Set("PlayCompletionSounds", chkCompletionSounds.Checked); Program.Settings.Set("ReportAfterMerge", chkMergeReport.Checked); Program.Settings.Set("ReportAfterPack", chkPackReport.Checked); diff --git a/WitcherScriptMerger/Inventory/InteractiveMergeRunner.cs b/WitcherScriptMerger/Inventory/InteractiveMergeRunner.cs index 6c7c8b2..69cd8c3 100644 --- a/WitcherScriptMerger/Inventory/InteractiveMergeRunner.cs +++ b/WitcherScriptMerger/Inventory/InteractiveMergeRunner.cs @@ -32,7 +32,7 @@ public InteractiveMergeRunner( ProgressChangedEventHandler progressHandler, RunWorkerCompletedEventHandler completedHandler) { - _fileMerger = new FileMerger(inventory, AppState.MergeEngine) + _fileMerger = new FileMerger(inventory) { OnMergeReport = ShowMergeReport, OnPackReport = ShowPackReport, diff --git a/WitcherScriptMerger/Program.cs b/WitcherScriptMerger/Program.cs index 47f2957..24ddfd6 100644 --- a/WitcherScriptMerger/Program.cs +++ b/WitcherScriptMerger/Program.cs @@ -14,7 +14,6 @@ using WitcherScriptMerger.Inventory; using WitcherScriptMerger.LoadOrder; using WitcherScriptMerger.Mcp; -using WitcherScriptMerger.Tools; namespace WitcherScriptMerger { @@ -66,20 +65,6 @@ public static MergeInventory Inventory [STAThread] static void Main(string[] args) { - // The default IMergeEngine implementation, supplied here since KDiff3MergeEngine - // needs Tools/KDiff3.cs's Win32 P/Invoke (host-only) - see Tools/IMergeEngine.cs. - // Must be set before anything calls Paths.ValidateDependencyPaths() or - // constructs a FileMerger, in any of the GUI/CLI/MCP paths below. The - // "MergeEngine" App.config setting can switch to DiffPlexMergeEngine (Core, no - // external binary) instead - not the default yet, since it hasn't been - // cross-checked against KDiff3 on enough real conflicting files (see CLAUDE.md - // and the PR that introduced it); this switch exists so it can be tried without - // recompiling, not as a signal that it's considered production-ready. - AppState.MergeEngine = - Settings.Get("MergeEngine").EqualsIgnoreCase("diffplex") - ? (IMergeEngine)new DiffPlexMergeEngine() - : new KDiff3MergeEngine(); - if (args.Length > 0) { Environment.ExitCode = RunCli(args); @@ -183,7 +168,7 @@ static int RunCli(string[] args) if (!Paths.ValidateDependencyPaths()) { Notifier.ShowError( - "A required dependency (KDiff3, QuickBMS, or wcc_lite) is missing. Configure its path " + + "A required dependency (QuickBMS or wcc_lite) is missing. Configure its path " + "in App.config, or run without arguments once to use the GUI's dependency setup."); return 1; } @@ -256,7 +241,7 @@ static int RunMcp() if (!Paths.ValidateDependencyPaths()) { Console.Error.WriteLine( - "A required dependency (KDiff3, QuickBMS, or wcc_lite) is missing. Configure its path in App.config."); + "A required dependency (QuickBMS or wcc_lite) is missing. Configure its path in App.config."); return 1; } diff --git a/WitcherScriptMerger/Tools/KDiff3.cs b/WitcherScriptMerger/Tools/KDiff3.cs deleted file mode 100644 index 2a76ad5..0000000 --- a/WitcherScriptMerger/Tools/KDiff3.cs +++ /dev/null @@ -1,302 +0,0 @@ -using System; -using System.Diagnostics; -using System.IO; -using System.Runtime.InteropServices; -using System.Text; -using System.Threading; -using WitcherScriptMerger.Inventory; - -namespace WitcherScriptMerger.Tools -{ - static class KDiff3 - { - public static string ExePath = Program.Settings.Get("KDiff3Path"); - - public static int Run( - FileMerger.MergeSource source1, - FileMerger.MergeSource source2, - FileInfo vanillaFile, - string outputPath) - { - if (!File.Exists(ExePath)) - { - Program.Notifier.ShowError("Can't find KDiff3 at this location:\n\n" + ExePath, "Missing KDiff3"); - return 1; - } - - var outputDir = Path.GetDirectoryName(outputPath); - - if (!Directory.Exists(outputDir)) - Directory.CreateDirectory(outputDir); - - var args = BuildArgs(source1, source2, vanillaFile, outputPath, out var hasVanillaVersion); - - if (!Program.Settings.Get("ReviewEachMerge") && hasVanillaVersion) - { - if (source1.TextFile.FullName.EqualsIgnoreCase(outputPath) - && source2.Hash != null && source2.Hash.IsOutdated) - { - Program.Notifier.ShowMessage( - "You are merging an updated mod file into a merge created with a previous version of the file.\n\n" + - "You should carefully inspect this merge, because KDiff3's auto-solving behavior KEEPS changes from the previous version of the mod file that have been REMOVED in the new version.", - "Warning", - NotifyButtons.OK, - DialogIcon.Warning); - } - else - args += " --auto"; - } - - var kdiff3Path = ResolveExePath(); - - var kdiff3Proc = Process.Start(kdiff3Path, args); - kdiff3Proc.WaitForExit(); - - return kdiff3Proc.ExitCode; - } - - public enum HeadlessResult { AutoSolved, NeedsManualResolution, Failed } - - // KDiff3 has no fail-fast mode - its own docs (doc/dothemerge.html) say plainly - // that when manual interaction is needed, a merge window opens, even in its own - // batch/automation mode. So this doesn't ask KDiff3 to behave headlessly; it - // launches it normally and detects a stuck merge itself: KDiff3 always briefly - // shows a plain "Conflicts" window on startup regardless of outcome (not a - // signal), but only a genuine unresolved conflict leaves open a second window - // titled " <-> [ <-> ] - KDiff3" - the actual comparison/merge - // editor. If that window is still open past a short grace period, this treats - // the merge as needing manual resolution, kills the process, and reports it as - // skipped rather than waiting on it (verified empirically against real and - // synthetic conflicts - see CLAUDE.md). Never writes to the real outputPath - // directly: KDiff3's -o target is a scratch path, only copied into place after - // a confirmed clean exit, so a killed process can never leave a partial file - // where the game would load it. - public static HeadlessResult RunHeadless( - FileMerger.MergeSource source1, - FileMerger.MergeSource source2, - FileInfo vanillaFile, - string outputPath) - { - if (!File.Exists(ExePath)) - { - Program.Notifier.ShowError("Can't find KDiff3 at this location:\n\n" + ExePath, "Missing KDiff3"); - return HeadlessResult.Failed; - } - - var scratchDir = Path.Combine(Paths.TempBundleContent, "HeadlessOutput"); - Directory.CreateDirectory(scratchDir); - var scratchOutputPath = Path.Combine(scratchDir, Guid.NewGuid().ToString("N") + Path.GetExtension(outputPath)); - - var args = BuildArgs(source1, source2, vanillaFile, scratchOutputPath, out var hasVanillaVersion); - - if (hasVanillaVersion - && source1.TextFile.FullName.EqualsIgnoreCase(outputPath) - && source2.Hash != null && source2.Hash.IsOutdated) - { - // The interactive path skips --auto here and relies on the user reviewing - // manually - there's nothing safe to do headlessly but skip it too. - Program.Notifier.ShowMessage( - $"Skipped {source1.Name} + {source2.Name}: merging an updated mod file into a merge " + - "created with a previous version needs manual review (KDiff3's auto-solving would keep " + - "changes from the previous version that have been removed in the new one).", - "Skipped", NotifyButtons.OK, DialogIcon.Warning); - return HeadlessResult.NeedsManualResolution; - } - args += " --auto"; - - // KDiff3's window can't be hidden or moved off-screen without KDiff3 hanging - // indefinitely instead of auto-solving - confirmed empirically against a hidden - // desktop, ShowWindow(SW_HIDE), and SetWindowPos off-screen, all three of which - // reliably broke it while an untouched window auto-solves normally. It also - // steals foreground focus while shown. Since it can't be suppressed, the best - // available mitigation is restoring focus to whatever had it beforehand once - // KDiff3's window is gone (auto-solved, failed, or killed) - see CLAUDE.md. - var previousForeground = NativeMethods.GetForegroundWindow(); - - var proc = Process.Start(ResolveExePath(), args); - var pid = proc.Id; - var sw = Stopwatch.StartNew(); - - try - { - const int gracePeriodMs = 3000; - const int backstopTimeoutMs = 60000; - long? mergeWindowFirstSeenMs = null; - - while (!proc.HasExited && sw.ElapsedMilliseconds < backstopTimeoutMs) - { - if (HasVisibleMergeWindow(pid)) - { - mergeWindowFirstSeenMs ??= sw.ElapsedMilliseconds; - if (sw.ElapsedMilliseconds - mergeWindowFirstSeenMs.Value > gracePeriodMs) - break; - } - else - { - mergeWindowFirstSeenMs = null; - } - proc.Refresh(); - Thread.Sleep(250); - } - - if (!proc.HasExited) - { - // Kill() only requests termination - wait for it to actually take effect - // before returning, so the finally block's focus restore isn't racing a - // window that's still technically alive (and might still own focus). - try { proc.Kill(entireProcessTree: true); proc.WaitForExit(2000); } catch { } - DeleteIfExists(scratchOutputPath); - Program.Notifier.ShowMessage( - $"Skipped {source1.Name} + {source2.Name}: needs manual conflict resolution.", - "Skipped", NotifyButtons.OK, DialogIcon.Warning); - return HeadlessResult.NeedsManualResolution; - } - - if (proc.ExitCode == 0 && File.Exists(scratchOutputPath)) - { - var outputDir = Path.GetDirectoryName(outputPath); - if (!Directory.Exists(outputDir)) - Directory.CreateDirectory(outputDir); - File.Copy(scratchOutputPath, outputPath, overwrite: true); - DeleteIfExists(scratchOutputPath); - return HeadlessResult.AutoSolved; - } - - DeleteIfExists(scratchOutputPath); - return HeadlessResult.Failed; - } - finally - { - RestoreForegroundWindow(previousForeground); - } - } - - // Plain SetForegroundWindow is denied by Windows' foreground-lock rules here: this - // process didn't own the foreground when KDiff3's window took over (KDiff3 did), so - // by the time this runs, this process isn't a privileged caller. Confirmed empirically - - // plain SetForegroundWindow was silently denied every time, even after waiting for - // KDiff3's process to fully exit. AttachThreadInput temporarily shares input state with - // whatever thread currently owns the foreground, which grants this thread the same - // privilege for the duration of the call - the standard workaround for this restriction. - // Still best-effort: if it fails, there's nothing destructive about not refocusing. - static void RestoreForegroundWindow(IntPtr previousForeground) - { - try - { - var currentForeground = NativeMethods.GetForegroundWindow(); - var foregroundThreadId = NativeMethods.GetWindowThreadProcessId(currentForeground, out _); - var currentThreadId = NativeMethods.GetCurrentThreadId(); - - var attached = foregroundThreadId != currentThreadId - && NativeMethods.AttachThreadInput(currentThreadId, foregroundThreadId, true); - try - { - NativeMethods.SetForegroundWindow(previousForeground); - } - finally - { - if (attached) - NativeMethods.AttachThreadInput(currentThreadId, foregroundThreadId, false); - } - } - catch { } - } - - static string BuildArgs( - FileMerger.MergeSource source1, - FileMerger.MergeSource source2, - FileInfo vanillaFile, - string outputPath, - out bool hasVanillaVersion) - { - hasVanillaVersion = (vanillaFile != null && vanillaFile.Exists); - - var vanillaPath = hasVanillaVersion ? FileEncoding.EnsureUtf16File(vanillaFile, "Vanilla") : null; - var source1Path = FileEncoding.EnsureUtf16File(source1.TextFile, "Source1"); - var source2Path = FileEncoding.EnsureUtf16File(source2.TextFile, "Source2"); - - var args = (hasVanillaVersion - ? "\"" + vanillaPath + "\" " - : ""); - - args += - $"\"{source1Path}\" \"{source2Path}\" " + - $"-o \"{outputPath}\" " + - "--cs \"WhiteSpace3FileMergeDefault=2\" " + - "--cs \"CreateBakFiles=0\" " + - "--cs \"LineEndStyle=1\" " + - "--cs \"FollowFileLinks=1\" " + - "--cs \"FollowDirLinks=1\""; - - if (!Program.Settings.Get("ShowPathsInKDiff3")) - { - if (hasVanillaVersion) - args += $" --L1 Vanilla --L2 \"{source1.Name}\" --L3 \"{source2.Name}\""; - else - args += $" --L1 \"{source1.Name}\" --L2 \"{source2.Name}\""; - } - - return args; - } - - static string ResolveExePath() - { - return Path.IsPathRooted(ExePath) - ? ExePath - : Path.Combine(Environment.CurrentDirectory, ExePath); - } - - static void DeleteIfExists(string path) - { - try { if (File.Exists(path)) File.Delete(path); } catch { } - } - - static bool HasVisibleMergeWindow(int pid) - { - var found = false; - NativeMethods.EnumWindows((hWnd, _) => - { - NativeMethods.GetWindowThreadProcessId(hWnd, out uint windowPid); - if (windowPid == (uint)pid && NativeMethods.IsWindowVisible(hWnd)) - { - var sb = new StringBuilder(256); - NativeMethods.GetWindowText(hWnd, sb, sb.Capacity); - if (sb.ToString().EndsWith(" - KDiff3", StringComparison.Ordinal)) - found = true; - } - return true; - }, IntPtr.Zero); - return found; - } - - static class NativeMethods - { - public delegate bool EnumWindowsProc(IntPtr hWnd, IntPtr lParam); - - [DllImport("user32.dll")] - public static extern bool EnumWindows(EnumWindowsProc enumProc, IntPtr lParam); - - [DllImport("user32.dll")] - public static extern uint GetWindowThreadProcessId(IntPtr hWnd, out uint processId); - - [DllImport("user32.dll")] - public static extern bool IsWindowVisible(IntPtr hWnd); - - [DllImport("user32.dll", CharSet = CharSet.Auto)] - public static extern int GetWindowText(IntPtr hWnd, StringBuilder lpString, int nMaxCount); - - [DllImport("user32.dll")] - public static extern IntPtr GetForegroundWindow(); - - [DllImport("user32.dll")] - public static extern bool SetForegroundWindow(IntPtr hWnd); - - [DllImport("kernel32.dll")] - public static extern uint GetCurrentThreadId(); - - [DllImport("user32.dll")] - public static extern bool AttachThreadInput(uint idAttach, uint idAttachTo, bool fAttach); - } - - } -} diff --git a/WitcherScriptMerger/Tools/KDiff3MergeEngine.cs b/WitcherScriptMerger/Tools/KDiff3MergeEngine.cs deleted file mode 100644 index 0d85783..0000000 --- a/WitcherScriptMerger/Tools/KDiff3MergeEngine.cs +++ /dev/null @@ -1,39 +0,0 @@ -using System.IO; -using WitcherScriptMerger.Inventory; - -namespace WitcherScriptMerger.Tools -{ - // The one real IMergeEngine implementation - see Core's Tools/IMergeEngine.cs for - // why this scaffolding exists. Just wraps the existing KDiff3.Run/RunHeadless - // calls FileMerger (now in Core) used to make directly; all the real logic - // (encoding normalization, window-persistence detection, focus restoration) stays - // in KDiff3.cs unchanged. - class KDiff3MergeEngine : IMergeEngine - { - public MergeEngineResult Merge( - FileMerger.MergeSource source1, - FileMerger.MergeSource source2, - FileInfo vanillaFile, - string outputPath) - { - var exitCode = KDiff3.Run(source1, source2, vanillaFile, outputPath); - return exitCode == 0 ? MergeEngineResult.AutoSolved : MergeEngineResult.Failed; - } - - public MergeEngineResult MergeHeadless( - FileMerger.MergeSource source1, - FileMerger.MergeSource source2, - FileInfo vanillaFile, - string outputPath) - { - return KDiff3.RunHeadless(source1, source2, vanillaFile, outputPath) switch - { - KDiff3.HeadlessResult.AutoSolved => MergeEngineResult.AutoSolved, - KDiff3.HeadlessResult.NeedsManualResolution => MergeEngineResult.NeedsManualResolution, - _ => MergeEngineResult.Failed, - }; - } - - public bool ValidateExePath() => File.Exists(KDiff3.ExePath); - } -} diff --git a/docs/decisions/kdiff3-retirement.md b/docs/decisions/kdiff3-retirement.md new file mode 100644 index 0000000..bd8ce46 --- /dev/null +++ b/docs/decisions/kdiff3-retirement.md @@ -0,0 +1,291 @@ +# Decision: Retire KDiff3 + +**Status:** Implemented. +**Type:** Architecture decision + historical record. + +## Decision + +KDiff3 is no longer a WSM dependency. `WitcherScriptMerger/Tools/KDiff3.cs`, +`WitcherScriptMerger/Tools/KDiff3MergeEngine.cs`, and the `IMergeEngine` +interface that used to sit between them and `FileMerger` are deleted. +`WitcherScriptMerger.Core/Tools/DiffPlexMergeEngine.cs` (in-process, built on +the DiffPlex NuGet package, no external binary) is now the sole and default +text-merge engine, called directly by `FileMerger` — see `CLAUDE.md`'s +"Interactive vs. headless split" section for the current architecture. + +This was the repo owner's explicit decision, not something arrived at by +default because DiffPlex happened to be "good enough." It trades away a real +capability: `DiffPlexMergeEngine` has a measured, non-zero failure rate on +dense multi-edit conflicts (see CLAUDE.md's Compatibility constraints for the +numbers) and no fallback for a vanilla-less merge, both of which +`KDiff3MergeEngine` handled — see "What a user's experience actually changes +to" below for what that means in practice. What KDiff3 offered in exchange — +a real interactive 3-way merge GUI, and a slightly more capable engine — came +at a real, ongoing cost: a GPL-licensed external binary dependency, a popup +window that steals foreground focus and can't be suppressed without hanging +the whole merge (see below), and roughly 250 lines of Win32 P/Invoke whose +correctness depended on undocumented, empirically-discovered timing behavior +in a third-party Qt application. That combination was judged not worth +carrying forward now that an in-process, dependency-free alternative exists, +even an imperfect one. + +## Why this document exists + +This is the only place this institutional knowledge survives. It was +originally written up in this fork's local, gitignored `HANDOFF.md`, and +lived operationally in `CLAUDE.md`'s "Compatibility constraints" section — but +`CLAUDE.md` describes the code as it exists today, and that section is being +removed along with the code it explained. Preserved here instead, before (in +the same change that) the motivating code is deleted, so a future reader +investigating "why does KDiff3 invocation look so strange" — or a future +attempt to reintroduce KDiff3 or a similar external GUI merge tool — has +something more durable than commit-message archaeology to start from. + +## What a user's experience actually changes to + +- **No more KDiff3 popup for a conflict that needs manual resolution.** + Previously, a genuine conflict opened KDiff3's own 3-way merge editor + window for the user to resolve by hand (interactive mode), or — in headless + CLI/MCP mode — briefly appeared, stole foreground focus, and was killed + after a timeout with the conflict reported as skipped. Now: a genuine + conflict writes a git/diff3-style conflict-marker sidecar file + (`<<<<<<<`/`|||||||`/`=======`/`>>>>>>>`, labeled with the real mod names) + and opens it in the OS's default associated editor for that file type + (`Tools/FileOpener.cs`, Core-side, `Process.Start` with + `UseShellExecute = true`) — in both interactive and headless modes, since + they're now the same code path underneath. There is no merge UI anymore; + resolving the conflict means editing the sidecar by hand (or opening the + three source files yourself) and re-running the merge once satisfied. +- **A small, measured, non-zero chance a real conflict is now reported as + "needs manual resolution" that KDiff3 might have auto-solved cleanly**, on + dense multi-edit conflicts specifically — see CLAUDE.md's Compatibility + constraints for the actual measured rates (from ~0.35% at realistic, + single-edit-per-side density up to double digits on adversarial dense + cases). This never produces silently wrong output — DiffPlexMergeEngine + detects the underlying DiffPlex bug that causes this and refuses to trust + the result rather than risk writing corrupted merge output — but it does + mean more conflicts now require the manual sidecar-editing workflow above + than would have under KDiff3. +- **No more vanilla-less 2-way fallback.** If no vanilla version of a file + can be found (expected mainly on the bundle-content path when no matching + vanilla bundle exists), `DiffPlexMergeEngine` refuses the merge outright + rather than attempting a degraded 2-way diff — KDiff3 had a coherent notion + of a 2-file merge and always attempted one in this situation; DiffPlex's + `ThreeWayDiffer`, as used here, does not, and attempting a 2-way fallback is + new scope this retirement didn't take on. +- **No more `ReviewEachMerge` or `ShowPathsInKDiff3` settings.** + `ReviewEachMerge` (show the merge UI even for an auto-solvable merge, to + double-check it) has no equivalent — there's no merge UI to show anymore. + `ShowPathsInKDiff3` (show real file paths instead of mod names in KDiff3's + `--L1/--L2/--L3` pane labels) doesn't cleanly transfer either: the closest + analogue, `DiffPlexMergeEngine`'s conflict-marker labels, already show mod + names (not paths) on the `<<<<<<<`/`>>>>>>>` lines, and a marker file read + as plain text just gets noisier with an absolute path where a short mod + name reads more clearly — so the setting and its checkbox were removed + rather than repurposed. +- **One fewer external binary to source separately.** `Paths.ValidateDependencyPaths()` + no longer checks a `KDiff3Path`; a fresh checkout only needs QuickBMS and + wcc_lite sourced separately to run end-to-end (see CLAUDE.md's "External + tool dependencies"). KDiff3 itself was GPL-licensed and safe to bundle into + a release (unlike QuickBMS/wcc_lite, which have no license file and were + never committed to source control) — that licensing question is now moot + for this dependency specifically. + +## Empirical findings preserved from `KDiff3.cs` + +The following was learned through direct, repeated empirical testing during +this fork's development — not from KDiff3's documentation, which (per +`doc/dothemerge.html`) says only that manual interaction always opens a +window, even in KDiff3's own "batch/automation mode," with no fail-fast or +truly headless option. This section is a preservation of that testing, kept +verbatim in substance from `CLAUDE.md`'s former "Compatibility constraints" +bullets and `KDiff3.cs`'s own comments, now that the code itself is gone. + +### Window-title polling: `"Conflicts"` vs. `" - KDiff3"` + +KDiff3 always briefly shows a plain window titled exactly `Conflicts` on +startup — regardless of whether the merge ultimately auto-solves or not. This +is **not** a "needs manual resolution" signal; it's transient and closes +within a few seconds either way. + +Only a genuine, unresolved conflict leaves open a **second** window, titled +` <-> [ <-> ] - KDiff3` (e.g. `Vanilla <-> modA <-> modB - +KDiff3`) — the actual comparison/merge editor. That window persists +indefinitely until a human closes it. An auto-solve's process, by contrast, +exits within a few seconds regardless of file size — 3400+ line files exited +in under 3 seconds in testing. + +`KDiff3.RunHeadless`'s detection logic (`HasVisibleMergeWindow`) used +`EnumWindows` + `GetWindowThreadProcessId` + `IsWindowVisible` + +`GetWindowText`, filtering to windows owned by KDiff3's own process ID and +checking `EndsWith(" - KDiff3", StringComparison.Ordinal)` — an ordinal, +suffix-only check, deliberately not matching on the transient `Conflicts` +title at all. + +The practical rule this produced: **detect on window persistence past a short +grace period (~2–3 seconds, to let the transient `Conflicts` window close), +never on elapsed time alone, and never by assuming a visible window means +failure.** `RunHeadless` used a 3000ms grace period +(`gracePeriodMs = 3000`) after first observing the `" - KDiff3"` window, +plus a 60-second backstop timeout (`backstopTimeoutMs = 60000`) as an +absolute ceiling regardless of window state. + +### The 250ms poll interval was load-bearing + +`RunHeadless` polled for the merge window every ~250ms +(`Thread.Sleep(250)`) between `EnumWindows` scans. This number was not +arbitrary or merely "fast enough to feel responsive" — it was discovered to +be a real constraint by accident, while testing window-suppression +techniques (see below): polling every 15ms for the first second — with +**zero** window manipulation, just read-only `EnumWindows`/`GetWindowText` +queries — reliably hung KDiff3 the same way the suppression techniques did. +The identical, untouched launch polled at 200ms auto-solved normally every +time. + +The likely mechanism: `GetWindowText` issues a cross-process +`SendMessage(WM_GETTEXT)` to the target window, which is a *blocking* call +that the target thread must service on its own message loop. Polling fast +enough plausibly starves or reorders KDiff3's own message loop during the +window it needs to actually compute the merge. "Poll faster to detect the +conflict window sooner" looks like an obviously-safe optimization and is not +one — it would silently turn every merge into a hang. + +### Five window-suppression techniques were tried; all either did nothing or broke the merge + +Tested empirically against both a guaranteed auto-solve case and a +guaranteed-conflict case (two synthetic mods editing the identical line +differently): + +| Technique | Result | +|---|---| +| `ProcessStartInfo.WindowStyle = Hidden` | Silently ignored by KDiff3/Qt — window shows full-size regardless. Doesn't break anything, doesn't hide anything either. | +| `ProcessStartInfo.WindowStyle = Minimized` | Also silently ignored (confirmed via `IsIconic`). Same as above. | +| `ShowWindow(hwnd, SW_HIDE)` | **Genuinely hides the window** — and reliably makes KDiff3 hang forever at the `Conflicts` splash instead of ever auto-solving. | +| `SetWindowPos` moved off-screen | Same: genuinely hides it, same reliable hang. | +| Launching on a separate, non-interactive Windows desktop (`CreateDesktop`) | Same: genuinely hides it, same reliable hang. | + +Control: the identical launch mechanism, completely untouched, auto-solved in +1.6–6.5 seconds every time. The pattern held across three independent +suppression mechanisms (`ShowWindow`, `SetWindowPos`, `CreateDesktop`) — a +strong signal that KDiff3's Qt runtime needs its window genuinely composited +on the real, interactive desktop to make progress at all, not something +fixable from outside the process. **Do not reintroduce any of these three +techniques without re-verifying they still hang** — and if a future KDiff3 +version changes this behavior, that verification needs to be redone before +trusting a different result. + +### Foreground focus theft, and the never-fully-verified restoration attempt + +KDiff3's window steals foreground focus while shown (confirmed via +`GetForegroundWindow()`). Given the suppression techniques above were a dead +end, `KDiff3.RunHeadless` instead accepted the window appearing and tried to +restore focus to whatever had it beforehand: + +1. Capture `previousForeground = GetForegroundWindow()` **before** launching + KDiff3. +2. Launch KDiff3, run the poll loop above. +3. In a `finally` block, once KDiff3's window is confirmed gone — the kill + path (`proc.Kill(entireProcessTree: true)`) is asynchronous, so it also + waited up to 2 seconds via `proc.WaitForExit(2000)` first, so the restore + attempt wouldn't race a window that was still technically alive — call + `RestoreForegroundWindow(previousForeground)`. + +`RestoreForegroundWindow` tried plain `SetForegroundWindow` first, which +Windows' foreground-lock policy **denied every single time** in testing: this +process never owned the foreground to begin with (KDiff3's window did), so by +the time it tried to reclaim it, it wasn't a privileged caller. It then +upgraded to the standard `AttachThreadInput` workaround — temporarily sharing +input state with whichever thread currently owns the foreground, calling +`SetForegroundWindow` under that shared state, then detaching — but this was +**also** observed denied in every test run in the sandboxed automation +environment used for that testing session. Whether that's a fundamental OS +limit in that scenario or an artifact of that specific environment's own +automation harness aggressively reclaiming focus was never resolved — it was +never tested from an ordinary, interactive desktop session. **This +restoration was never verified to actually work in practice; it was always, +at best, a good-faith best-effort mitigation, not a proven one.** + +### Invoke via `Process.Start(fileName, argsString)` directly, never through a shell + +A prior verification pass tested KDiff3 invocation through Git Bash/MSYS2 and +concluded a real conflicting file (`damageManagerProcessor.ws`) still needed +manual GUI resolution even after fixing an unrelated encoding mismatch. +Re-tested later through .NET's own `Process.Start(fileName, argsString)` two- +string overload (`UseShellExecute = false` by default on modern .NET) — the +actual code path WSM used — the identical file, both raw and +encoding-normalized, auto-solved cleanly every time. The bash-based test had +been an invocation-environment artifact, not real KDiff3 behavior. Any future +tool invoked the way KDiff3 was should be tested through the actual +`Process.Start` overload the app uses, not through an interactive shell, +before drawing conclusions about its behavior. + +### Encoding normalization + +Vanilla `.ws` files are UTF-16LE with a BOM; mod authors' files are often +plain UTF-8/ASCII with no BOM. KDiff3 had no command-line flag to specify +per-input encoding, and a mismatch could make it treat an entire file as +unmatchable, falling back to manual GUI resolution instead of auto-solving — +confirmed against a real file, `baseEffect.ws`, which failed to auto-solve +with mismatched encodings and succeeded cleanly, with correct merged output, +once both inputs were normalized to UTF-16LE+BOM. This normalization +requirement outlived KDiff3 itself — `Tools/FileEncoding.cs` (Core) still +normalizes every merge engine's input the same way, since the underlying +reason (matching vanilla's own encoding so the game will load a merged file +at all) has nothing to do with which merge engine is active. + +### Command-line flags used + +For reference, `KDiff3.BuildArgs` invoked KDiff3 with: + +``` +"" "" "" -o "" +--cs "WhiteSpace3FileMergeDefault=2" +--cs "CreateBakFiles=0" +--cs "LineEndStyle=1" +--cs "FollowFileLinks=1" +--cs "FollowDirLinks=1" +[--L1 Vanilla --L2 "" --L3 ""] +[--auto] +``` + +`WhiteSpace3FileMergeDefault=2` (verified against KDiff3's own source, not +assumed) means "always pick input B" for a conflict that's purely +whitespace once whitespace differences are ignored — given KDiff3's file +order of vanilla/source1/source2 mapping to inputs A/B/C, input B is always +`source1` (the first mod in merge order). `LineEndStyle=1` means DOS-style +(`\r\n`) line endings, matching vanilla `.ws` files. Both of these semantics +were carried forward deliberately into `DiffPlexMergeEngine.BuildMerge`, +which mirrors them (whitespace-only conflicts auto-resolve to the first mod's +side verbatim; synthetic conflict-marker lines use `\r\n`) — see +`DiffPlexMergeEngine.cs`'s own comments for where each of these still applies +today. `--auto` was appended only for non-interactive (headless, or +interactive-without-`ReviewEachMerge`) runs. + +### Scratch-output-then-copy pattern + +`RunHeadless`'s `-o` target was never the real output path directly — it was +always a scratch path under `Paths.TempBundleContent\HeadlessOutput\.`, only copied to the real output path after a **confirmed clean +exit** (`proc.ExitCode == 0 && File.Exists(scratchOutputPath)`). A killed +process (the "needs manual resolution, timed out" case) could therefore never +leave a partial or corrupt file at the real output path — worst case, nothing +happened at all, and the conflict was reported as skipped. This same +"never touch the real output except on confirmed success" principle carries +forward into `DiffPlexMergeEngine.MergeHeadless`, which never writes to +`outputPath` on anything other than a clean auto-solve, and routes conflict +markers to a separate sidecar location precisely so a failed/retried merge +can never be mistaken for a completed one (see `DiffPlexMergeEngine.cs`'s +comment on `GetConflictMarkerPath`). + +## What was *not* carried forward, and why + +- **A real interactive merge UI.** DiffPlex has no equivalent to KDiff3's + 3-way merge editor. The conflict-marker-sidecar-plus-default-editor + workflow (see above) is the replacement, and it is a strictly less + guided experience — there's no pane-by-pane visual diff, no click-to-choose + resolution, just a text file with git-style markers. This was accepted as + part of the retirement decision, not an oversight. +- **The vanilla-less 2-way fallback.** See "What a user's experience + actually changes to" above. +- **Any window-suppression or focus-management logic.** None of it is needed + anymore, since DiffPlexMergeEngine never opens a window in the first place. diff --git a/docs/vortex-extension-design.md b/docs/vortex-extension-design.md index c95674d..22e0236 100644 --- a/docs/vortex-extension-design.md +++ b/docs/vortex-extension-design.md @@ -1,5 +1,14 @@ # Vortex Extension Design (Unit 4) +**Note (post-KDiff3-retirement):** this document describes WSM's architecture as of +its own writing, when KDiff3 was still a required on-disk dependency alongside +QuickBMS/wcc_lite. KDiff3 has since been retired in favor of an in-process, +external-binary-free merge engine — see `docs/decisions/kdiff3-retirement.md`. This +materially *simplifies* several points below that treated KDiff3/QuickBMS/wcc_lite as a +combined packaging problem (e.g. §2.2, the "KDiff3/QuickBMS/wcc_lite are a separate +problem" point) — only QuickBMS/wcc_lite remain. Not otherwise updated inline below; +read the KDiff3-specific mentions that follow as historical context, not current fact. + **Status: design document only.** Nothing in this file is implemented. There is no TypeScript/Node scaffolding anywhere in this repository, and this unit does not add any — that work is explicitly deferred to a later, separate implementation batch. This From 8b38725b0e11a0181d54755a241ae3569e6c7a8a Mon Sep 17 00:00:00 2001 From: Chris Knight Date: Fri, 7 Aug 2026 18:37:32 -0400 Subject: [PATCH 2/3] Fix code review findings from KDiff3 retirement - DiffPlexMergeEngine.NormalizeWhitespace used the parameterless string.Trim(), which trims the full Unicode whitespace category (including NBSP) at the edges of the joined comparison text, silently undoing this class's own documented reason for using a narrow ASCII-only regex elsewhere in the same method. A trailing-NBSP-vs-plain-text conflict could be misclassified as whitespace-only and silently discarded. Fixed via Trim(WhitespaceChars), the same explicit ASCII set as the regex; regression test added, and manually verified to fail against the pre-fix code before being restored. - The interactive path's "Merge N of M was canceled" prompt is now shown for every automatic engine refusal (genuine conflict, missing vanilla, outdated hash), not just true user cancellations - DiffPlexMergeEngine's interactive Merge() has no UI to cancel out of at all, unlike the retired KDiff3 engine. Renamed to ConfirmContinueAfterSkippedMerge and reworded to "was skipped", which is accurate for both this method's call sites. - FileOpener.Open's return value was discarded, so the notifier message unconditionally claimed "attempting to open it now for review" even when the sidecar's .conflict extension has no OS file association and the open silently failed. Reordered so the open happens first and the message reflects what actually happened. - OptionsForm.Designer.cs: removing two checkboxes shrank grpMerging.Size but left the button row and ClientSize unchanged, opening a 57px dead-space gap where the original had an 11px one. Cascaded the 46px reduction through the button row and ClientSize to restore the original gap. - Removed a fabricated citation: a comment and CLAUDE.md both claimed CONTRIBUTING.md documents a "no premature abstraction" convention that justified deleting IMergeEngine. It doesn't - grep confirms CONTRIBUTING.md never mentions abstraction, interfaces, or "premature". Reworded to state this as the deletion's own reasoning instead of a false citation. - Fixed a stale comment in BuildMerge still pointing at a Program.cs engine-selection switch this same change deleted. - Mcp/CLAUDE.md's "Minimal required permissions" section didn't mention that merge_conflicts can launch external processes (FileOpener.Open, once per genuine conflict, uncapped) or that DiffPlexConflictsDirectory is a filesystem root the process writes to - both real omissions in a doc whose stated purpose is "exactly what the process touches, and at what privilege level". - Tightened an overclaiming test-suite comment about xunit's parallelization defaults: sequential execution is only guaranteed within one collection (implicitly one per class), not across classes - a future test class also exercising the shared FileOpener.Open static could race this one. Two findings from the same review were evaluated and deliberately not acted on: Program.TryOpenFile has the identical missing-UseShellExecute bug this change's FileOpener.cs documents but doesn't fix (pre-existing, affects 7 untestable GUI call sites, out of scope for this diff); and DiffPlexMergeEngine becoming the sole engine without new verification to close its already- disclosed reliability gap is this change's own stated premise, not an oversight (see docs/decisions/kdiff3-retirement.md). Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01GXAuGMLB44T5Zv5o5ZzKah --- CLAUDE.md | 2 +- .../Inventory/FileMerger.cs | 40 ++++++++-- WitcherScriptMerger.Core/Mcp/CLAUDE.md | 19 ++++- .../Tools/DiffPlexMergeEngine.cs | 80 ++++++++++++++----- .../Tools/DiffPlexMergeEngineTests.cs | 41 +++++++++- .../Forms/OptionsForm.Designer.cs | 8 +- 6 files changed, 151 insertions(+), 39 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index c16bea4..6ff2a64 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -55,7 +55,7 @@ Folder map — **host**: Core's `FileMerger` never sees a `TreeNode`, `BackgroundWorker`, or `Forms.*` type. Its headless methods (`MergeConflictsHeadless` et al.) are unchanged in shape from before the split. Its interactive methods (`MergeFilesInteractive`, `MergeFlatFileInteractive`, `MergeBundleFileInteractive`, `MergeTextInteractive`) take a plain `InteractiveMergeRequest` (relative path, bundle flag, vanilla file path, ordered `MergeSource[]`) instead of `TreeNode[]`, and report back through `OnMergeReport`/`OnPackReport` callbacks instead of constructing `MergeReportForm`/`PackReportForm` directly. The host's `Inventory/InteractiveMergeRunner.cs` is the thing `MainForm` actually talks to: it extracts `InteractiveMergeRequest`s from checked `TreeNode`s (inside its `BackgroundWorker`'s `DoWork`, matching the pre-split threading model — extracting outside `DoWork` let a bad node throw synchronously on the UI thread instead of being captured by `BackgroundWorker`), owns the `BackgroundWorker`, and supplies the `OnMergeReport`/`OnPackReport` callbacks (report forms, completion sounds). -Both `FileMerger.MergeText*` methods talk to a private `DiffPlexMergeEngine` field (`Merge`/`MergeHeadless`) constructed by `FileMerger`'s own constructor. There used to be an `IMergeEngine` interface between them, with two implementations — `KDiff3MergeEngine` (host, wrapping `Tools/KDiff3.Run`/`KDiff3.RunHeadless`) and `DiffPlexMergeEngine` (Core) — selectable via a `MergeEngine` App.config key. KDiff3 was retired (see `docs/decisions/kdiff3-retirement.md` for the full rationale and the empirical KDiff3 process-behavior findings preserved there); with only one implementation ever going to remain, the interface indirection was deleted along with it, per this repo's own "no premature abstraction" convention (see CONTRIBUTING.md) — `FileMerger` now calls `DiffPlexMergeEngine` directly. +Both `FileMerger.MergeText*` methods talk to a private `DiffPlexMergeEngine` field (`Merge`/`MergeHeadless`) constructed by `FileMerger`'s own constructor. There used to be an `IMergeEngine` interface between them, with two implementations — `KDiff3MergeEngine` (host, wrapping `Tools/KDiff3.Run`/`KDiff3.RunHeadless`) and `DiffPlexMergeEngine` (Core) — selectable via a `MergeEngine` App.config key. KDiff3 was retired (see `docs/decisions/kdiff3-retirement.md` for the full rationale and the empirical KDiff3 process-behavior findings preserved there); with only one implementation ever going to remain, the interface indirection was deleted along with it as premature abstraction (an interface with exactly one implementation for its whole remaining life) — not a documented repo-wide rule (CONTRIBUTING.md doesn't state one; an earlier version of this sentence claimed it did, which code review caught as a false citation), just this deletion's own reasoning, matching what `IMergeEngine.cs`'s own former doc comment already said about itself. `FileMerger` now calls `DiffPlexMergeEngine` directly. **`DiffPlexMergeEngine`** (Core, `Tools/DiffPlexMergeEngine.cs`) is the sole text-merge engine — in-process, built on the DiffPlex NuGet package (MIT-licensed), needing no external binary. It builds its own merge loop around `DiffPlex.ThreeWayDiffer.CreateDiffs` rather than calling `ThreeWayDiffer.CreateMerge` directly, so it can intercept `ThreeWayChangeType.Conflict` blocks itself: a conflict whose two sides are equal once whitespace is collapsed (joined-and-collapsed comparison over the classic ASCII whitespace set — space/tab/CR/LF/form-feed/vertical-tab, deliberately narrower than .NET regex's Unicode-aware `\s` so a genuine content difference that happens to be NBSP-vs-space isn't misclassified as whitespace-only — not per-line `Trim()`, and never applied when either side has zero pieces, since a real deletion must never be conflated with "the surviving side happens to collapse to empty too") auto-resolves by taking the first mod's side verbatim, mirroring the now-retired KDiff3 engine's `--cs "WhiteSpace3FileMergeDefault=2"` (confirmed against the KDiff3 source, preserved in `docs/decisions/kdiff3-retirement.md`: value 2 means "always pick input B", and KDiff3's own file order — vanilla, source1, source2 — maps source1 to B). A genuine conflict instead produces git/diff3-style conflict markers (`<<<<<<< ` / `||||||| Vanilla` / `=======` / `>>>>>>> `) written to a **sidecar** file under `Paths.DiffPlexConflictsDirectory` (a dedicated top-level `DiffPlexConflicts` folder — via `GetConflictMarkerPath`, keyed by an `XxHash32` of the full output path plus its filename) rather than to `outputPath` itself — writing markers to `outputPath` would make `FileMerger`'s pre-merge `File.Exists(_outputPath)` overwrite guard treat it as an already-completed merge and permanently skip retrying, since `HeadlessMergeNotifier` always declines the overwrite prompt. The sidecar went through two prior locations before landing here, each ruled out by direct end-to-end testing against the real CLI rather than by inspection alone: it originally lived right beside `outputPath` (`.conflict`), which code review flagged for two real problems (a flat-file conflict's `outputPath` sits inside the live `Paths.ModsDirectory` tree, which nothing ever cleans up; a bundle-content conflict's `outputPath` sits inside `Paths.MergedBundleContent`, which `Tools/WccLite.PackBundle` packs *wholesale* with no filtering, so a leftover `.conflict` file there could get embedded as bogus content into a later-packed `blob0.bundle`); it then moved under `Paths.TempBundleContent`, which fixed both of those but broke immediately in end-to-end testing for a third reason neither review nor unit tests caught - `FileMerger.CleanUpTempFiles()` deletes the entire `TempBundleContent` tree wholesale at the end of every headless merge run (to clear QuickBMS-unpacked bundle scratch content), so the sidecar was gone by the time the CLI process exited, before a user could ever see it. `Paths.DiffPlexConflictsDirectory` is an unrelated top-level name specifically to avoid that collision - see its own comment in `Paths.cs` for the full story. A conflict-marker file that later becomes an auto-solve on retry has its stale sidecar deleted before writing the fresh output. diff --git a/WitcherScriptMerger.Core/Inventory/FileMerger.cs b/WitcherScriptMerger.Core/Inventory/FileMerger.cs index 07aef66..6dfaad1 100644 --- a/WitcherScriptMerger.Core/Inventory/FileMerger.cs +++ b/WitcherScriptMerger.Core/Inventory/FileMerger.cs @@ -214,7 +214,7 @@ void MergeFlatFileInteractive(InteractiveMergeRequest file, Merge merge, bool is { source1 = MergeSource.FromFlatFile(mergedFile, null); } - else if (!ConfirmContinueAfterCanceledMerge(file.OrderedSources.Length - i - 1, merge)) + else if (!ConfirmContinueAfterSkippedMerge(file.OrderedSources.Length - i - 1, merge)) break; } @@ -244,7 +244,7 @@ void MergeBundleFileInteractive(InteractiveMergeRequest file, Merge merge, bool if (!GetUnpackedFiles(file.RelativePath, ref source1, ref source2)) { - if (ConfirmContinueAfterCanceledMerge(file.OrderedSources.Length - i - 1, merge)) + if (ConfirmContinueAfterSkippedMerge(file.OrderedSources.Length - i - 1, merge)) continue; break; } @@ -254,7 +254,7 @@ void MergeBundleFileInteractive(InteractiveMergeRequest file, Merge merge, bool { source1 = MergeSource.FromFlatFile(mergedFile, null); } - else if (!ConfirmContinueAfterCanceledMerge(file.OrderedSources.Length - i - 1, merge)) + else if (!ConfirmContinueAfterSkippedMerge(file.OrderedSources.Length - i - 1, merge)) break; } @@ -318,10 +318,38 @@ bool ConfirmRemainingConflict(string mergedModName) } // Returns false when the caller should stop trying further merges for this - // file (user declined to continue past a canceled/failed merge). - bool ConfirmContinueAfterCanceledMerge(int remainingMergesForFile, Merge merge) + // file (user declined to continue past a skipped/failed merge). + // + // Named/worded around "skipped", not "canceled", after code review caught a real + // mislabeling: this fires whenever MergeTextInteractive returns null, which used + // to mean "the user canceled out of KDiff3's GUI" (a true cancellation, since + // KDiff3's interactive path really did hand control to the user) but now, with + // DiffPlexMergeEngine, means "the engine automatically refused this pairing" + // (genuine conflict, missing vanilla file, outdated-hash guard, or a caught + // DiffAlgorithmException) - DiffPlexMergeEngine.Merge() has no UI at all, so there + // is no longer any user action for "canceled" to describe here. The engine + // already showed its own explanatory modal (via AppState.Notifier) before + // returning, so this second prompt only needs to ask whether to continue with any + // remaining merges for the file - describing what already happened as "skipped" + // keeps that prompt accurate instead of misattributing an automatic refusal to + // the user. + // + // When remainingMergesForFile is 0, this shows a bare OK-only acknowledgment with + // no real decision attached (there's nothing left to continue to) - back-to-back + // with DiffPlexMergeEngine's own explanatory modal for a MergeTextInteractive + // failure, that's a genuinely redundant second dialog. Deliberately not + // special-cased away, though: this method has a second call site + // (MergeBundleFileInteractive, on a GetUnpackedFiles failure) where nothing else + // shows any explanatory message first - GetUnpackedFiles itself is silent on + // failure - so this modal is the ONLY acknowledgment the user gets in that case. + // Suppressing it whenever remainingMergesForFile is 0 would fix the redundant + // case but silently drop the only feedback in the other one; distinguishing them + // would need this method to know which failure path it's covering, which isn't + // worth the extra plumbing just to save one OK click in the already-explained + // case. + bool ConfirmContinueAfterSkippedMerge(int remainingMergesForFile, Merge merge) { - var msg = $"Merge {ProgressInfo.CurrentMergeNum} of {ProgressInfo.TotalMergeCount} was canceled."; + var msg = $"Merge {ProgressInfo.CurrentMergeNum} of {ProgressInfo.TotalMergeCount} was skipped."; var buttons = NotifyButtons.OK; if (remainingMergesForFile > 0) { diff --git a/WitcherScriptMerger.Core/Mcp/CLAUDE.md b/WitcherScriptMerger.Core/Mcp/CLAUDE.md index 3df1d7f..f8266b2 100644 --- a/WitcherScriptMerger.Core/Mcp/CLAUDE.md +++ b/WitcherScriptMerger.Core/Mcp/CLAUDE.md @@ -6,9 +6,16 @@ that section doesn't: exactly what the process touches, and at what privilege le ## Minimal required permissions -- **Standard user-level file I/O only.** No admin/elevated rights are needed to run any of - the four tools. -- **Three filesystem roots, all ordinary user-writable locations:** +- **Standard user-level file I/O — plus process spawning for conflict review.** No + admin/elevated rights are needed to run any of the four tools, but `merge_conflicts` + is not I/O-only: `DiffPlexMergeEngine.MergeHeadless` calls `Tools/FileOpener.Open` + (`Process.Start` with `UseShellExecute = true`) once per genuinely-conflicting file it + processes, launching whatever the OS has associated with that sidecar's file type — + there is no cap, batching, or opt-out on the *number* of conflicts merged in one call + (only `dryRun` suppresses the open entirely, see below). A `merge_conflicts` call with + no `relativePaths` filter against a mods folder with many genuine conflicts can + therefore open that many windows on the host desktop in one call. +- **Four filesystem roots, all ordinary user-writable locations:** - The configured mods directory (`Paths.ModsDirectory`) and game directory (`Paths.GameDirectory`) — read for scanning conflicts and vanilla/mod source files, write for merged output (flat-file merges land inside the mods directory; a bundle @@ -22,6 +29,12 @@ that section doesn't: exactly what the process touches, and at what privilege le executable itself lives in, regardless of what directory an MCP client launches it from — verified empirically (a client-supplied working directory had no effect; `MergeInventory.xml` always landed next to the executable). + - `Paths.DiffPlexConflictsDirectory` (`DiffPlexConflicts`, next to the executable for + the same `Environment.CurrentDirectory` reason as the previous bullet) — every + genuine conflict `merge_conflicts` processes writes a git/diff3-style conflict-marker + sidecar file here (see the root `CLAUDE.md`'s "Interactive vs. headless split" + section), even for a `dryRun` call. Nothing sweeps this directory automatically, + unlike `TempBundleContent`. - **`merge_conflicts`'s `relativePaths` and `orderOverrides` keys are validated against `Paths.ModsDirectory`** before any scan or merge runs (`WsmMcpTools.EnsureInScope` / `IsWithinModsDirectory`) — an entry that doesn't resolve inside that directory (absolute diff --git a/WitcherScriptMerger.Core/Tools/DiffPlexMergeEngine.cs b/WitcherScriptMerger.Core/Tools/DiffPlexMergeEngine.cs index ab69b46..434a88a 100644 --- a/WitcherScriptMerger.Core/Tools/DiffPlexMergeEngine.cs +++ b/WitcherScriptMerger.Core/Tools/DiffPlexMergeEngine.cs @@ -35,9 +35,14 @@ public enum MergeEngineResult // a text-merge engine without referencing Tools/KDiff3.cs's Win32 P/Invoke, which had // to stay in the host project; now that KDiff3MergeEngine is gone, DiffPlexMergeEngine // (already Core-side, same as FileMerger) is the only implementation there will ever - // be, so the interface no longer bridges anything - keeping it would be exactly the - // premature "abstraction with one implementation for its whole remaining life" this - // repo's own conventions say to avoid (see CLAUDE.md/CONTRIBUTING.md). + // be, so the interface no longer bridges anything - keeping it would be premature + // abstraction (an interface with exactly one implementation for its whole remaining + // life). This isn't a documented repo-wide rule - a prior version of this comment + // claimed CONTRIBUTING.md said as much, which code review caught as false (grep + // confirms CONTRIBUTING.md never mentions abstraction, interfaces, or "premature" at + // all) - it's this deletion's own reasoning, consistent with IMergeEngine.cs's own + // former doc comment, which called itself "scaffolding... NOT meant as a permanent + // pluggable-engine abstraction" and predicted its own removal once KDiff3 was gone. // // There's no UI here at all, so "interactive" and "headless" collapse to the same // underlying logic: Merge() just runs MergeHeadless() and maps NeedsManualResolution @@ -92,6 +97,20 @@ public DiffAlgorithmException(string message) : base(message) { } // are excluded. Flagged in code review, see CLAUDE.md. static readonly Regex WhitespaceRun = new Regex(@"[ \t\r\n\f\v]+", RegexOptions.Compiled); + // Same exact character set as WhitespaceRun above, as a char[] rather than a + // regex - used by NormalizeWhitespace's own Trim() call. A real bug, caught in + // code review: NormalizeWhitespace used to call the parameterless string.Trim(), + // which trims by char.IsWhiteSpace - the full Unicode whitespace category, + // including NBSP - silently undoing this class's whole stated reason for using a + // narrow ASCII-only regex in the first place, but only at the leading/trailing + // edges of the joined, collapsed text (WhitespaceRun itself was always correctly + // ASCII-only for internal runs). Concretely: oldPieces=["Hello "], + // newPieces=["Hello"] collapse to the same "Hello" after Trim() strips the edge + // NBSP, silently auto-resolving as whitespace-only a case that should stay a + // genuine conflict - exactly the NBSP-vs-space content-loss scenario the comment + // above already warned about, just missed for the edges specifically. + static readonly char[] WhitespaceChars = { ' ', '\t', '\r', '\n', '\f', '\v' }; + #endregion public MergeEngineResult Merge( @@ -240,27 +259,37 @@ public MergeEngineResult MergeHeadless( // they read it later from a different working directory than the one that // wrote it - the absolute form is unambiguous regardless of when/where it's read. var sidecarFullPath = Path.GetFullPath(sidecarPath); - var openSuffix = openConflictMarkers - ? " - attempting to open it now for review." - : " - not opened automatically (dry run preview)."; + + // Opened BEFORE the notifier message, deliberately reordered from an earlier + // version of this method (code review caught the problem with the original + // order): the message needs to say whether the file actually opened, and + // FileOpener.Open's own bool return is exactly that signal - a stock machine + // with no default association for ".conflict" makes Process.Start either + // throw ERROR_NO_ASSOCIATION (swallowed by FileOpener.TryOpen's own catch, + // returning false) or raise the OS "how do you want to open this?" picker, and + // the message would otherwise unconditionally claim "attempting to open it + // now" regardless of what actually happened. Best-effort either way: a failed + // open doesn't change the result below, the sidecar is on disk regardless. + // Skipped entirely for a dry run (openConflictMarkers = false - see this + // method's parameter comment), which also means dryRun's message always uses + // the "open it manually" wording, never claims an open that was never + // attempted. One consequence of this ordering worth stating plainly: on the + // GUI's interactive path, AppState.Notifier.ShowMessage is a real blocking + // modal, so the editor (if any) now opens BEHIND that modal instead of after + // it - arguably better (the file's already up by the time the user dismisses + // the dialog) but a deliberate change from this method's original "acknowledge + // the skip, then the editor opens" sequencing, not an accident. + var opened = openConflictMarkers && FileOpener.Open(sidecarPath); + var openSuffix = !openConflictMarkers + ? " - open it manually to review (dry run preview)." + : opened + ? " - opened it for review." + : " - open it manually to review."; AppState.Notifier.ShowMessage( $"Skipped {source1.Name} + {source2.Name}: genuine conflict, needs manual resolution. " + $"Conflict markers were written to {sidecarFullPath}{openSuffix}", "Skipped", NotifyButtons.OK, DialogIcon.Warning); - // Runs after the notifier message (which blocks on the GUI's interactive - // path, via a real modal MessageBox - not here, headless) so the two read as - // one coherent sequence: acknowledge the skip, then the editor opens. Fires - // identically whether this method was reached via the CLI/MCP headless path - // directly or via the GUI's interactive Merge(), which just delegates to this - // method (see this class's header comment) - one mechanism for both, not two. - // Best-effort: FileOpener.Open never throws, and a failed open (no file - // association, etc.) doesn't change the result below - the sidecar is on disk - // either way. Skipped entirely for a dry run (openConflictMarkers = false) - - // see this method's parameter comment. - if (openConflictMarkers) - FileOpener.Open(sidecarPath); - return MergeEngineResult.NeedsManualResolution; } @@ -392,8 +421,13 @@ public static MergeTextResult BuildMerge(string baseText, string oldText, string // rates of both failure modes, this is caught here (an exception) and // verified for (the silent case, via the post-loop count check below) rather // than trusted - MergeHeadless treats either as "needs manual resolution" - // rather than ever risking corrupted merge output. This is also a primary - // reason DiffPlexMergeEngine isn't the default engine yet (see Program.cs). + // rather than ever risking corrupted merge output. This measured, non-zero + // failure rate is the primary reason retiring KDiff3 in favor of this engine + // was a deliberate tradeoff, not a strict improvement - see + // docs/decisions/kdiff3-retirement.md. (There used to be a separate + // engine-selection switch in Program.cs that kept this engine non-default + // specifically because of this gap - that switch and KDiff3 itself are both + // gone now; this engine is the sole engine, gap and all.) try { foreach (var block in diffResult.DiffBlocks) @@ -559,7 +593,9 @@ static bool IsWhitespaceOnlyDifference(IReadOnlyList oldPieces, IReadOnl static string NormalizeWhitespace(IEnumerable pieces) { - return WhitespaceRun.Replace(string.Concat(pieces), " ").Trim(); + // Trim(WhitespaceChars), not the parameterless Trim() - see WhitespaceChars' + // own comment for the real NBSP-related bug this guards against. + return WhitespaceRun.Replace(string.Concat(pieces), " ").Trim(WhitespaceChars); } #endregion diff --git a/WitcherScriptMerger.Tests/Tools/DiffPlexMergeEngineTests.cs b/WitcherScriptMerger.Tests/Tools/DiffPlexMergeEngineTests.cs index 9e45640..c7107da 100644 --- a/WitcherScriptMerger.Tests/Tools/DiffPlexMergeEngineTests.cs +++ b/WitcherScriptMerger.Tests/Tools/DiffPlexMergeEngineTests.cs @@ -26,9 +26,18 @@ namespace WitcherScriptMerger.Tests.Tools // Any fixture that reaches a genuine-conflict outcome now also reaches // FileOpener.Open (see MergeHeadless's comment) - those fixtures swap it for a stub // in a try/finally around the real FileOpener.TryOpen, so no test run ever actually - // launches a process. All fixtures in this class run sequentially (xunit's default: - // one implicit collection per test class), so swapping this process-wide static field - // per-test is safe without extra locking. + // launches a process. FileOpener.Open is a single process-wide static field, so + // swapping it per-test is only safe because nothing else touches it concurrently - + // fixtures within this one class run sequentially (xunit's default: no [Collection] + // attribute here, so this class is its own implicit collection, and collections, not + // individual test classes, are xunit's actual unit of parallelism), but a *different* + // test class that also exercises DiffPlexMergeEngine.MergeHeadless/Merge on a + // genuine-conflict fixture would be a different collection by that same default, and + // could run concurrently with this one, racing this same static field. If a future + // test class needs to do that, it needs either an explicit shared, non-parallel + // [Collection] with this one, or its own equivalent stub-and-restore discipline + // guaranteed not to overlap with this class's - don't assume the sequencing this + // class relies on extends automatically to a second class touching the same static. public class DiffPlexMergeEngineTests { [Fact] @@ -188,6 +197,32 @@ public void BuildMerge_DeletionVersusWhitespaceReformat_IsNotMisclassifiedAsWhit Assert.Contains(">>>>>>> modB", result.MergedText); } + [Fact] + public void BuildMerge_TrailingNbspVersusPlainSpace_IsNotMisclassifiedAsWhitespaceOnly() + { + // Regression test for a real bug caught in code review: NormalizeWhitespace + // used to call the parameterless string.Trim(), which (unlike the + // WhitespaceRun regex used for internal runs) trims by the full Unicode + // whitespace category - including NBSP (U+00A0) - at the leading/trailing + // edges of the joined, collapsed comparison text. That silently undid this + // class's own stated NBSP-preservation guarantee (see WhitespaceRun's own + // comment) whenever the differing NBSP happened to land at a piece boundary, + // which a trailing one on the last differing line always does. One mod's line + // ends in a real NBSP (plausible in localized dialogue text); the other's the + // same line without it - a genuine content difference that must stay a + // conflict, not silently collapse to "equal" once the trailing NBSP is + // (incorrectly) trimmed away along with the line's own \r\n. + var baseText = "x = 1;\r\n"; + var oldText = "x = 2;\u00A0\r\n"; + var newText = "x = 2;\r\n"; + + var result = DiffPlexMergeEngine.BuildMerge(baseText, oldText, newText, "modA", "modB"); + + Assert.True(result.HasConflicts); + Assert.Contains("<<<<<<< modA", result.MergedText); + Assert.Contains(">>>>>>> modB", result.MergedText); + } + [Fact] public void MergeHeadless_EncodingMismatch_NormalizesAndProducesUtf16LEWithBomOutput() { diff --git a/WitcherScriptMerger/Forms/OptionsForm.Designer.cs b/WitcherScriptMerger/Forms/OptionsForm.Designer.cs index 22c434c..cf7d177 100644 --- a/WitcherScriptMerger/Forms/OptionsForm.Designer.cs +++ b/WitcherScriptMerger/Forms/OptionsForm.Designer.cs @@ -253,7 +253,7 @@ private void InitializeComponent() // btnCancel // this.btnCancel.DialogResult = System.Windows.Forms.DialogResult.Cancel; - this.btnCancel.Location = new System.Drawing.Point(108, 442); + this.btnCancel.Location = new System.Drawing.Point(108, 396); this.btnCancel.Name = "btnCancel"; this.btnCancel.Size = new System.Drawing.Size(90, 25); this.btnCancel.TabIndex = 12; @@ -264,7 +264,7 @@ private void InitializeComponent() // btnOK // this.btnOK.DialogResult = System.Windows.Forms.DialogResult.Cancel; - this.btnOK.Location = new System.Drawing.Point(12, 442); + this.btnOK.Location = new System.Drawing.Point(12, 396); this.btnOK.Name = "btnOK"; this.btnOK.Size = new System.Drawing.Size(90, 25); this.btnOK.TabIndex = 11; @@ -275,7 +275,7 @@ private void InitializeComponent() // btnApply // this.btnApply.DialogResult = System.Windows.Forms.DialogResult.Cancel; - this.btnApply.Location = new System.Drawing.Point(204, 442); + this.btnApply.Location = new System.Drawing.Point(204, 396); this.btnApply.Name = "btnApply"; this.btnApply.Size = new System.Drawing.Size(90, 25); this.btnApply.TabIndex = 13; @@ -289,7 +289,7 @@ private void InitializeComponent() this.AutoScaleDimensions = new System.Drawing.SizeF(6F, 13F); this.AutoScaleMode = System.Windows.Forms.AutoScaleMode.Font; this.CancelButton = this.btnCancel; - this.ClientSize = new System.Drawing.Size(306, 478); + this.ClientSize = new System.Drawing.Size(306, 432); this.ControlBox = false; this.Controls.Add(this.btnApply); this.Controls.Add(this.btnCancel); From e5fd87d49c9ac0a2c01eda232e169665bfe3f6c2 Mon Sep 17 00:00:00 2001 From: Chris Knight Date: Fri, 7 Aug 2026 18:44:40 -0400 Subject: [PATCH 3/3] Fix claims-vs-code mismatches from a second review pass - CLAUDE.md's "Interactive vs. headless split" section still described the sidecar-open behavior in its pre-reorder sequencing (open after the notifier message) and never mentioned dryRun's suppression of it. Updated to match the actual current order and document openConflictMarkers. - The reordering's own code comment claimed FileOpener.Open's return value is "exactly" a signal that an editor opened. It isn't - it only distinguishes Process.Start succeeding from throwing. This change's own end-to-end verification observed a real OpenWith.exe spawn (the OS's file-open picker, since the test machine has no association for ".conflict"), meaning "opened" can be true while what the user sees is a picker, not an editor. Reworded to say what the value actually distinguishes, using that same observed case as a concrete example instead of an unqualified claim. - CLAUDE.md's Project overview claimed origin was still the upstream AnotherSymbiote/WitcherScriptMerger remote and no separate fork existed. False - a separate fork (TheValiantOne/WitcherScriptMerger, default branch main) is the actual origin; upstream (AnotherSymbiote, default branch master) is a second remote kept for reference. This stale claim caused a real mistake: `gh pr create` run without --repo/--base for this same change defaulted to opening a PR against the upstream repo's master branch instead of this fork's main (closed immediately once caught). Corrected, with the mistake called out explicitly so it isn't repeated. - Minor: a code comment's NBSP example used a literal U+00A0 character (visually indistinguishable from a space), rather than an explicit \u00A0 escape - fixed for the same reason the regression test itself avoids a literal character. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01GXAuGMLB44T5Zv5o5ZzKah --- CLAUDE.md | 4 +- .../Tools/DiffPlexMergeEngine.cs | 45 ++++++++++++------- 2 files changed, 31 insertions(+), 18 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 6ff2a64..04d39dd 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -6,7 +6,7 @@ This file provides guidance to Claude Code (claude.ai/code) when working with co Script Merger for The Witcher 3 — a Windows desktop tool (WinForms, not WPF) that detects and merges conflicting mod script files. It scans a mod folder, finds `.ws`/`.xml` files (including inside `.bundle` packages) that multiple mods modify, and drives a 3-way merge (vanilla + mod1 + mod2) via an in-process DiffPlex-based merge engine (`Tools/DiffPlexMergeEngine.cs`, Core). `.bundle` package contents are unpacked with QuickBMS and repacked with wcc_lite. KDiff3 was formerly used for text merges instead — it was retired; see `docs/decisions/kdiff3-retirement.md` for why and for the empirical KDiff3 process-behavior findings preserved there now that the code is gone. -This is a fork of the upstream `AnotherSymbiote/WitcherScriptMerger` repo (still the `origin` remote — no separate fork exists yet), currently mid-modernization. See `HANDOFF.md` at the repo root for the full rationale behind the fork and detailed gotchas hit during the .NET modernization — read it before picking up follow-on work in this repo. Of its original list of open goals, whitespace/diff-noise and a CLI mode (see "CLI mode" below) are done; dependency-packaging/licensing decisions are still open. An MCP server mode (see "MCP mode" below) was added afterward, beyond that original list, to let an MCP client (e.g. Claude Code) drive merges directly instead of only through the CLI. +This is a fork of the upstream `AnotherSymbiote/WitcherScriptMerger` repo, currently mid-modernization. A separate fork, `TheValiantOne/WitcherScriptMerger`, now exists and is this project's actual `origin` remote (default branch `main`) — `upstream` is a second configured remote pointing at `AnotherSymbiote/WitcherScriptMerger` (default branch `master`), kept for reference/pulling upstream changes, not as a PR target. This distinction is load-bearing for tooling, not just trivia: an earlier version of this sentence claimed no separate fork existed and that `origin` was still the upstream repo - false by the time it was written, and it cost a real mistake when it caused `gh pr create` (run with no `--repo`/`--base`) to silently default to opening a PR against `AnotherSymbiote/WitcherScriptMerger`'s `master` instead of this fork's `main`, visible in that unrelated repo's history until closed. Always pass `--repo TheValiantOne/WitcherScriptMerger --base main` explicitly (or otherwise confirm the target) when opening a PR from this repo. See `HANDOFF.md` at the repo root for the full rationale behind the fork and detailed gotchas hit during the .NET modernization — read it before picking up follow-on work in this repo. Of its original list of open goals, whitespace/diff-noise and a CLI mode (see "CLI mode" below) are done; dependency-packaging/licensing decisions are still open. An MCP server mode (see "MCP mode" below) was added afterward, beyond that original list, to let an MCP client (e.g. Claude Code) drive merges directly instead of only through the CLI. ## Build & run @@ -59,7 +59,7 @@ Both `FileMerger.MergeText*` methods talk to a private `DiffPlexMergeEngine` fie **`DiffPlexMergeEngine`** (Core, `Tools/DiffPlexMergeEngine.cs`) is the sole text-merge engine — in-process, built on the DiffPlex NuGet package (MIT-licensed), needing no external binary. It builds its own merge loop around `DiffPlex.ThreeWayDiffer.CreateDiffs` rather than calling `ThreeWayDiffer.CreateMerge` directly, so it can intercept `ThreeWayChangeType.Conflict` blocks itself: a conflict whose two sides are equal once whitespace is collapsed (joined-and-collapsed comparison over the classic ASCII whitespace set — space/tab/CR/LF/form-feed/vertical-tab, deliberately narrower than .NET regex's Unicode-aware `\s` so a genuine content difference that happens to be NBSP-vs-space isn't misclassified as whitespace-only — not per-line `Trim()`, and never applied when either side has zero pieces, since a real deletion must never be conflated with "the surviving side happens to collapse to empty too") auto-resolves by taking the first mod's side verbatim, mirroring the now-retired KDiff3 engine's `--cs "WhiteSpace3FileMergeDefault=2"` (confirmed against the KDiff3 source, preserved in `docs/decisions/kdiff3-retirement.md`: value 2 means "always pick input B", and KDiff3's own file order — vanilla, source1, source2 — maps source1 to B). A genuine conflict instead produces git/diff3-style conflict markers (`<<<<<<< ` / `||||||| Vanilla` / `=======` / `>>>>>>> `) written to a **sidecar** file under `Paths.DiffPlexConflictsDirectory` (a dedicated top-level `DiffPlexConflicts` folder — via `GetConflictMarkerPath`, keyed by an `XxHash32` of the full output path plus its filename) rather than to `outputPath` itself — writing markers to `outputPath` would make `FileMerger`'s pre-merge `File.Exists(_outputPath)` overwrite guard treat it as an already-completed merge and permanently skip retrying, since `HeadlessMergeNotifier` always declines the overwrite prompt. The sidecar went through two prior locations before landing here, each ruled out by direct end-to-end testing against the real CLI rather than by inspection alone: it originally lived right beside `outputPath` (`.conflict`), which code review flagged for two real problems (a flat-file conflict's `outputPath` sits inside the live `Paths.ModsDirectory` tree, which nothing ever cleans up; a bundle-content conflict's `outputPath` sits inside `Paths.MergedBundleContent`, which `Tools/WccLite.PackBundle` packs *wholesale* with no filtering, so a leftover `.conflict` file there could get embedded as bogus content into a later-packed `blob0.bundle`); it then moved under `Paths.TempBundleContent`, which fixed both of those but broke immediately in end-to-end testing for a third reason neither review nor unit tests caught - `FileMerger.CleanUpTempFiles()` deletes the entire `TempBundleContent` tree wholesale at the end of every headless merge run (to clear QuickBMS-unpacked bundle scratch content), so the sidecar was gone by the time the CLI process exited, before a user could ever see it. `Paths.DiffPlexConflictsDirectory` is an unrelated top-level name specifically to avoid that collision - see its own comment in `Paths.cs` for the full story. A conflict-marker file that later becomes an auto-solve on retry has its stale sidecar deleted before writing the fresh output. -**The sidecar is opened for the user, not just written.** Immediately after writing it and reporting the skip via `AppState.Notifier.ShowMessage`, `MergeHeadless` calls `Tools/FileOpener.Open` (a swappable static `Func`, defaulting to `Process.Start` with `UseShellExecute = true` so it resolves the OS's file association rather than trying to execute the sidecar as a process image) on the sidecar path — opening it in the user's default editor for that file type. Since there's no UI here at all, `Merge()` (interactive) just runs `MergeHeadless()` and maps `NeedsManualResolution` to `Failed` — this also means the sidecar-write-and-open behavior, and the "merging an updated mod file into an existing merge chain" outdated-hash guard, both fire identically whether reached via the GUI's interactive path or the CLI/MCP headless path, since it's the exact same underlying call either way. Deliberately not `Program.TryOpenFile` (the host's existing, WinForms-adjacent equivalent, used elsewhere for opening merged output files): that helper's non-`.exe` branch is a bare `Process.Start(path)` with no `UseShellExecute = true`, which on modern .NET (unlike .NET Framework, where `UseShellExecute` defaulted to true) throws for a non-executable path — silently swallowed by that method's surrounding `catch`, and out of scope to fix there, but not a pattern worth propagating into this new code path. `Tools/FileOpener.cs` exists specifically so both the GUI-interactive and CLI/MCP-headless paths can reach a correctly-implemented version of this without Core referencing `System.Windows.Forms`. +**The sidecar is opened for the user, not just written.** `MergeHeadless` writes the sidecar, then (unless `openConflictMarkers` is `false` - see below) calls `Tools/FileOpener.Open` (a swappable static `Func`, defaulting to `Process.Start` with `UseShellExecute = true` so it resolves the OS's file association rather than trying to execute the sidecar as a process image) on the sidecar path, and only then reports the skip via `AppState.Notifier.ShowMessage` - the open happens *before* the message, not after (an earlier version of this method did it the other way around; code review caught that the message needs `FileOpener.Open`'s own bool return to say whether the file actually opened, which requires calling it first). That bool distinguishes only "`Process.Start` succeeded" from "`Process.Start` threw" (e.g. `ERROR_NO_ASSOCIATION`) - it is not a guarantee an editor actually came up. Confirmed empirically during this feature's own end-to-end verification: on the machine used for testing, the sidecar's `.conflict` extension had no registered file association, so `Process.Start` succeeded by launching `OpenWith.exe` (the OS's own "how do you want to open this file?" picker) - `opened` was `true`, and the message correctly said "opened it for review" by its own definition of "opened" (the call didn't fail), even though what the user actually sees is a picker dialog, not directly an editor. `Merge()` (interactive) just runs `MergeHeadless()` and maps `NeedsManualResolution` to `Failed` since there's no UI here at all - this also means the sidecar-write-and-open behavior, and the "merging an updated mod file into an existing merge chain" outdated-hash guard, both fire identically whether reached via the GUI's interactive path or the CLI/MCP headless path, since it's the exact same underlying call either way. `MergeHeadless`'s `openConflictMarkers` parameter (default `true`) is the one thing that differs by caller: `FileMerger.MergeTextHeadless` passes `openConflictMarkers: !dryRun`, so a dry run (`MergeConflictsHeadless(dryRun: true)`, including the MCP `merge_conflicts` tool's `dryRun` option) still writes a genuine conflict's sidecar - unchanged, pre-existing behavior - but never launches anything for it, since a preview must not have that kind of side effect; a real (non-dry-run) run still opens every genuine conflict's sidecar with no cap on how many. Deliberately not `Program.TryOpenFile` (the host's existing, WinForms-adjacent equivalent, used elsewhere for opening merged output files): that helper's non-`.exe` branch is a bare `Process.Start(path)` with no `UseShellExecute = true`, which on modern .NET (unlike .NET Framework, where `UseShellExecute` defaulted to true) throws for a non-executable path — silently swallowed by that method's surrounding `catch`, and out of scope to fix there, but not a pattern worth propagating into this new code path. `Tools/FileOpener.cs` exists specifically so both the GUI-interactive and CLI/MCP-headless paths can reach a correctly-implemented version of this without Core referencing `System.Windows.Forms`. Two loose ends worth stating plainly rather than leaving implicit: nothing automatically deletes the `DiffPlexConflicts` directory itself (the same "accumulates until manually cleared" property `TempBundleContent` has, and the CLI-mode section below already tells users to clear that one between runs - `DiffPlexConflicts` needs the same manual housekeeping, just without an automated sweep); and `Paths.DiffPlexConflictsDirectory` is a relative path, resolved against `Environment.CurrentDirectory` - `Program.RunCli` sets that to `AppContext.BaseDirectory` before anything else runs (see "Startup flow" below), so in CLI mode sidecars land predictably next to the installed exe, but the GUI path never does that reset, so a GUI-mode conflict's sidecar lands wherever the process's CWD happened to be at launch (typically the exe's own directory too, for a normal double-click launch, but not guaranteed). diff --git a/WitcherScriptMerger.Core/Tools/DiffPlexMergeEngine.cs b/WitcherScriptMerger.Core/Tools/DiffPlexMergeEngine.cs index 434a88a..1fc5168 100644 --- a/WitcherScriptMerger.Core/Tools/DiffPlexMergeEngine.cs +++ b/WitcherScriptMerger.Core/Tools/DiffPlexMergeEngine.cs @@ -104,7 +104,10 @@ public DiffAlgorithmException(string message) : base(message) { } // including NBSP - silently undoing this class's whole stated reason for using a // narrow ASCII-only regex in the first place, but only at the leading/trailing // edges of the joined, collapsed text (WhitespaceRun itself was always correctly - // ASCII-only for internal runs). Concretely: oldPieces=["Hello "], + // ASCII-only for internal runs). Concretely: oldPieces=["Hello\u00A0"] (a literal + // U+00A0 NBSP, written out explicitly here since it's visually indistinguishable + // from a plain space in most editors/diffs - the same reason the regression test + // for this uses the same \u00A0 escape rather than a literal character), // newPieces=["Hello"] collapse to the same "Hello" after Trim() strips the edge // NBSP, silently auto-resolving as whitespace-only a case that should stay a // genuine conflict - exactly the NBSP-vs-space content-loss scenario the comment @@ -129,13 +132,16 @@ public MergeEngineResult Merge( // dryRun parameter): a dry run's whole contract is "preview only, no side effects // a user didn't ask for" (the MCP merge_conflicts tool's own dryRun description // promises no merged output, bundle repack, or MergeInventory.xml write), and - // FileOpener.Open launching a real editor/process is exactly that kind of surprise - // side effect for an operation whose entire point is to be inspectable without - // consequence. The conflict-marker sidecar itself is still written either way - // (pre-existing behavior, not something this parameter changes) - only the - // auto-open is conditional, since that's the specific side effect that turns a - // preview into something visibly disruptive (an editor window popping up per - // conflict for a mods folder with many of them). + // FileOpener.Open launching a real process is exactly that kind of surprise side + // effect for an operation whose entire point is to be inspectable without + // consequence - whatever the OS resolves that launch to, an editor if ".conflict" + // has an association or its own "how do you want to open this?" picker if not + // (confirmed both are possible - see MergeHeadless's own comment below). The + // conflict-marker sidecar itself is still written either way (pre-existing + // behavior, not something this parameter changes) - only the auto-open is + // conditional, since that's the specific side effect that turns a preview into + // something visibly disruptive (a window popping up per conflict for a mods + // folder with many of them). public MergeEngineResult MergeHeadless( FileMerger.MergeSource source1, FileMerger.MergeSource source2, @@ -262,14 +268,21 @@ public MergeEngineResult MergeHeadless( // Opened BEFORE the notifier message, deliberately reordered from an earlier // version of this method (code review caught the problem with the original - // order): the message needs to say whether the file actually opened, and - // FileOpener.Open's own bool return is exactly that signal - a stock machine - // with no default association for ".conflict" makes Process.Start either - // throw ERROR_NO_ASSOCIATION (swallowed by FileOpener.TryOpen's own catch, - // returning false) or raise the OS "how do you want to open this?" picker, and - // the message would otherwise unconditionally claim "attempting to open it - // now" regardless of what actually happened. Best-effort either way: a failed - // open doesn't change the result below, the sidecar is on disk regardless. + // order): the message needs FileOpener.Open's own bool return to pick its + // wording, which requires calling it first. That bool distinguishes only + // "Process.Start succeeded" from "Process.Start threw" (e.g. + // ERROR_NO_ASSOCIATION, swallowed by FileOpener.TryOpen's own catch, returning + // false) - it is NOT a guarantee an editor actually came up. Confirmed + // empirically during this feature's own end-to-end verification: on the + // machine used for testing, ".conflict" had no registered file association, so + // Process.Start succeeded by launching OpenWith.exe (the OS's own "how do you + // want to open this file?" picker) - opened came back true, and the message + // below says "opened it for review" by its own definition of "opened" (the + // call didn't fail), even though what the user actually sees is a picker + // dialog, not directly an editor. Still strictly more honest than an + // unconditional "attempting to open it now" regardless of outcome, which is + // what this message used to say. Best-effort either way: a failed open doesn't + // change the result below, the sidecar is on disk regardless. // Skipped entirely for a dry run (openConflictMarkers = false - see this // method's parameter comment), which also means dryRun's message always uses // the "open it manually" wording, never claims an open that was never