Retire KDiff3; make DiffPlexMergeEngine the sole merge engine

CONTRIBUTING.md

@@ -19,22 +19,22 @@ 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/<short-description>` for new functionality, `fix/<short-description>` for bug fixes, `chore/<short-description>` 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
 
 This repository is developed with AI coding agents (Claude Code, and expect others), openly — that's not hidden, and it's not discouraged. `CLAUDE.md` carries the operational guidance these tools use when working in this repo, kept up to date as the codebase changes; read it before pointing an agent at this repo. If these guidelines are silent on something and you're using an agent, defer to the explicit rules below over whatever the agent proposes on its own.
 
 - **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.

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.**

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() { }
 	}
 }

WitcherScriptMerger.Core/Cli/MergeOperations.cs

@@ -32,10 +32,7 @@ public static FileMerger.HeadlessMergeSummary RunMerge(
 			IReadOnlyDictionary<string, string[]> 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);
 		}
 	}

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<Merge> _pendingBundleMerges = new List<Merge>();
 
 		#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();
 		}
 
@@ -221,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;
 			}
 
@@ -251,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;
 				}
@@ -261,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;
 			}
 
@@ -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;
@@ -326,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)
 			{
@@ -611,7 +631,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;