Skip to content

Split into WitcherScriptMerger.Core + host GUI/CLI/MCP project - #5

Merged
TheValiantOne merged 4 commits into
mainfrom
feature/split-core-project
Aug 7, 2026
Merged

Split into WitcherScriptMerger.Core + host GUI/CLI/MCP project#5
TheValiantOne merged 4 commits into
mainfrom
feature/split-core-project

Conversation

@TheValiantOne

@TheValiantOne TheValiantOne commented Aug 7, 2026

Copy link
Copy Markdown
Owner

Summary

Splits the single WinForms project into a cross-platform WitcherScriptMerger.Core class library (net10.0, no WinForms reference) plus the existing GUI/CLI/MCP host project (net10.0-windows7.0, unchanged WinExe/UseWindowsForms). This is Unit 5 of the modernization batch — the foundational unit everything else (DiffPlex merge engine, test project, MCP hardening, Linux host, KDiff3 removal) builds on top of.

Moved into Core, unchanged in behavior: FileIndex/, Inventory/ (including FileMerger, see below), LoadOrder/, Tools/Hasher.cs + QuickBms.cs + WccLite.cs, Paths.cs, AppSettings.cs, Cli/, Mcp/. Left in the host: Forms/, Controls/, Tools/KDiff3.cs (Win32 P/Invoke), Program.cs, App.config, Properties/.

Disclosure: this PR was produced with AI assistance (Claude Code), per this repo's CONTRIBUTING.md.

Design decisions

The task brief anticipated most of this shape, but several things had to be resolved that weren't spelled out explicitly — flagging them here since a reviewer shouldn't have to re-derive them from the diff.

AppState — the circular-dependency problem the brief didn't mention

Program.Notifier / Program.Settings / Program.LoadOrder / Program.Inventory were static fields on the host's Program class. Almost every file moving to Core reads or writes them (Paths.ValidateModsDirectory, AppSettings's own error reporting, CustomLoadOrder, ModFileIndex, FileMerger, Cli/MergeOperations, Mcp/WsmMcpTools...). Core can never reference the host assembly, so those fields couldn't stay on Program as originally declared.

Fix: a new WitcherScriptMerger.AppState static class in Core now owns Notifier/Settings/LoadOrder/Inventory, plus a new MergeEngine (see below). Program.cs's Notifier/Settings/LoadOrder/Inventory became pass-through properties forwarding to AppState, so every existing host call site (Program.Notifier.ShowError(...), Program.Inventory = ..., etc.) needed zero changes — only files that moved into Core needed the mechanical Program.XAppState.X rename. AppState has an explicit (empty) static constructor to suppress beforefieldinit, so its field-initializer ordering (Notifier before Settings, matching the original Program.cs declaration order — AppSettings's constructor calls Notifier.ShowError on a missing config file) stays deterministic, and MaybeAttachConsole() still runs before anything can report a startup error. Verified empirically: renaming App.config away and running merge from a terminal still prints "Config file is missing." to that terminal (see Verification below).

A related instance of the same problem: Paths.ValidateDependencyPaths() used to call File.Exists(KDiff3.ExePath) directly, but KDiff3.cs stays host-only. Fixed by adding IMergeEngine.ValidateExePath() (implemented by KDiff3MergeEngine as File.Exists(KDiff3.ExePath)) so Paths (Core) validates the merge engine's dependency through the same AppState.MergeEngine seam instead of reaching into Tools/KDiff3.cs directly.

IMergeNotifier neutralization

  • Added NotifyResult/NotifyButtons/DialogIcon in Core (NotifyTypes.cs), 1:1 with the DialogResult/MessageBoxButtons/MessageBoxIcon members actually in use (buttons: the full 6-value set HeadlessMergeNotifier already handled defensively; icons: only the 6 values real call sites pass — None, Warning, Error, Exclamation, Information, Question).
  • ShowModal(Form) was dropped from the interface entirely, not just neutralized. I grepped every call site first: all 5 (FileMerger's three report-form popups, MainForm's Dependencies/Options menu commands) are GUI-only, interactive-only code, and none of the headless (MergeConflictsHeadless/CLI/MCP) paths ever called it. Since Form has no cross-platform equivalent and nothing outside the host actually needs this method, it stays as a plain (non-interface) public method on MainForm, and the interactive-only host code (InteractiveMergeRunner) calls Program.MainForm.ShowModal(...) directly instead of going through the notifier abstraction. HeadlessMergeNotifier's old defensive ShowModal stub (for "in case a call site is missed") is gone too, since that's now structurally impossible — nothing in Core can call a method Core's interface doesn't have.
  • MainForm.ShowMessage/ShowError now implement the neutral interface and translate to/from real MessageBox.Show(...)/DialogResult via ToNative(NotifyButtons)/ToNative(DialogIcon)/ToNeutral(DialogResult) helpers.
  • LoadOrderValidator.PromptToPrioritizeMergedMod used a direct MessageBox.Show(...) with a MessageBoxManager-customized "Ne&ver" Cancel-button caption and MessageBoxDefaultButton.Button2. Per the task brief's explicit instruction, this now routes through IMergeNotifier.ShowMessage(...) instead. This does lose the custom "Never" caption (shows plain "Cancel") — a real but purely cosmetic loss, brief-sanctioned. The default-button-2 ("No") focus is not lost, despite my first pass here mischaracterizing it as an equally-acceptable cosmetic loss too — code review correctly called that out as a real safety regression (an accidental Enter/Space keypress could silently rewrite the user's real mods.settings), so I added it back properly: see "Code review pass" below for the defaultResult parameter that preserves it. Note for the reviewer: a parallel Wave-0 unit may also be touching this exact file (a LoadOrderValidator fix) — expect a possible merge conflict there.

IMergeEngine — scaffolding, not a permanent abstraction

Two methods rather than a single method with an interactive bool flag: Merge(...) (mirrors KDiff3.Run, may block on the tool's own UI) and MergeHeadless(...) (mirrors KDiff3.RunHeadless, never blocks, can report NeedsManualResolution). This maps 1:1 onto the two existing KDiff3 entry points instead of inventing a boolean-flag branch inside one method. KDiff3MergeEngine (host) is a thin wrapper — KDiff3.cs itself is unchanged except three enum-literal swaps (MessageBoxButtons/MessageBoxIconNotifyButtons/DialogIcon) and dropping its now-unneeded using System.Windows.Forms;. None of the window-detection/poll-interval/focus-restoration logic CLAUDE.md flags as load-bearing was touched.

As the task brief anticipated, this is explicitly scaffolding for this wave only — a later unit deleting KDiff3 will likely delete this interface too and inline the replacement into FileMerger, unless a test project ends up depending on it as a seam.

FileMerger split shape

Core's FileMerger keeps: the MergeSource/HeadlessMergeSummary types (unchanged), the already-headless methods (MergeConflictsHeadless et al., now calling MergeEngine.MergeHeadless instead of KDiff3.RunHeadless directly), shared helpers (ConfirmOutputOverwrite, GetUnpackedFiles, UnpackFile, PackNewBundle, cleanup), and new interactive orchestration methods (MergeFilesInteractive, MergeFlatFileInteractive, MergeBundleFileInteractive, MergeTextInteractive, ConfirmRemainingConflict, ConfirmContinueAfterCanceledMerge) that mirror the old MergeByTreeNodesAsync/MergeFlatFileNode/MergeBundleFileNode/MergeText/HandleCanceledMerge almost line-for-line, but driven by a new InteractiveMergeRequest (relative path, bundle flag, vanilla file path, ordered MergeSource[]) instead of TreeNode[].

I considered instead having the host adapter convert TreeNode selections into the same shape MergeConflictsHeadless already consumes and reusing that method for both paths, since it's less code. I didn't, because the interactive and headless paths have real behavioral differences that would otherwise have to be re-introduced as special cases: per-pairwise-merge report popups (MergeText/now MergeTextInteractive calls the report callback once per successful merge in a multi-mod chain, not once per file), the ConfirmRemainingConflict load-order gate (interactive-only), and ConfirmContinueAfterCanceledMerge's ask-to-continue-or-abort semantics (also interactive-only, no headless equivalent). Keeping the interactive orchestration as its own method group made preserving that exact behavior straightforward instead of threading interactive-only branches through the headless method.

ConfirmRemainingConflict's gate check reads OrderedSources[i].Name (derived via ModFile.GetModNameFromPath, which falls back to Paths.MergedBundleContent for a source that's an intermediate bundle-merge result rather than an original per-mod file). I initially added a separate, explicitly-carried OrderedModNames string array out of caution about that fallback, since it looked fragile at a glance. Code review (see below) correctly pointed out the gate check always runs before any element of OrderedSources is touched by the pairwise merge loop, so the fallback case can never actually apply there — I removed the redundant field rather than keep two sources of truth for the same data.

Neither System.Media.SystemSounds nor any Forms.* type appears in Core's FileMerger — both moved to the host's new InteractiveMergeRunner (WitcherScriptMerger/Inventory/InteractiveMergeRunner.cs), which:

  • Owns the BackgroundWorker (Core's FileMerger no longer has one).
  • Extracts InteractiveMergeRequest[] from checked TreeNodes (the one and only place TreeNode.GetMetadata() is called for this flow).
  • Exposes MergeByTreeNodesAsync/RepackBundleAsync with the same signatures the old FileMerger had, so MainForm.cs's call sites only needed new FileMerger(...)new InteractiveMergeRunner(...) — nothing else in MainForm.cs changed for this.
  • Supplies OnMergeReport/OnPackReport callbacks that construct MergeReportForm/PackReportForm, play the completion sound (PlayCompletionSounds setting), and call Program.MainForm.ShowModal(...) — i.e., exactly what the old inline using (var reportForm = ...) { ShowModal } blocks did, just relocated.
  • Kept the if (_bgWorker.IsBusy) throw guard from the original RepackBundleAsync.

Deliberate minor deviation: the shared ShowPackReport callback sets ProgressInfo.CurrentAction = "Showing pack report" before showing PackReportForm. The original MergeByTreeNodesAsync's inline pack-report block did this; the original RepackBundleAsync's inline pack-report block did not. Unifying both call sites' report-handling into one callback means both flows get the label now — a harmless, arguably-corrective behavior change (a progress label visible for a fraction of a second right before the dialog appears), flagged here rather than silently folded in.

Extensions.cs split

Extensions.cs's pure string helpers (EqualsIgnoreCase, StartsWithIgnoreCase, GetPluralS, etc.) are used throughout code that moved to Core, but the file as a whole has WinForms TreeNode/TreeView P/Invoke helpers that must stay host-side. Split into Core's StringExtensions.cs (same WitcherScriptMerger namespace, different class name to avoid a duplicate-type error across the two assemblies — call sites are unaffected since these are extension methods resolved by namespace, not by class name) and the host's Extensions.cs (WinForms-only content, System.Text.RegularExpressions/using System;-only-where-still-needed trimmed).

Verification

dotnet build WitcherScriptMerger.sln: succeeds. WitcherScriptMerger.Core.csproj has zero references to System.Windows.Forms anywhere (confirmed by grep — the only hits are in comments explaining why there aren't any) and, since Core's TFM is bare net10.0 with no UseWindowsForms, any such reference would fail to compile at all, not just warn. WitcherScriptMerger.csproj builds with the same 5 pre-existing CA1823 warnings that exist on main today (MessageBoxManager.cs, Program.cs, SMTree.cs — all unrelated to this change), zero new warnings. dotnet format whitespace WitcherScriptMerger.sln --verify-no-changes passes.

Manual E2E verification used a scratch game/mods tree under the OS temp dir (two synthetic mods, modA/modB, both editing the same line of a shared test.ws script differently — a guaranteed conflict) with placeholder quickbms.exe/witcher3.bms/wcc_lite.exe files (CheckBundleContents=false, so the flat-file path under test genuinely never touches either) and, in place of the real (GPL, not available in this sandbox) KDiff3 binary, a small stub KDiff3.exe (a dotnet publish-produced console app) that parses -o <path> out of its argv and writes a canned "merged" file there, exiting 0. This exercises the real, unmodified Tools/KDiff3.cs code path end to end — Process.Start, EnsureUtf16Encoding, BuildArgs, exit-code handling, and for RunHeadless the window-detection loop (no window ever appears, so it reports AutoSolved almost immediately) — the only thing not real is KDiff3's own diff algorithm, which this PR does not touch.

  • CLI (WitcherScriptMerger.exe merge, run from a working directory different from the exe's own): first run exit code 0, Merged 1 file(s), skipped 0.; re-running against the same (now-existing) output exit code 2, Merged 0 file(s), skipped 1. (the safe non-destructive HeadlessMergeNotifier default for the resulting "Overwrite?" prompt) — both are the documented existing behavior, now running through AppState/IMergeEngine. MergeInventory.xml's schema is unchanged (<MergeInventory><Merge><RelativePath>.../<IncludedMod Hash="...">name</IncludedMod>.../<MergedModName>...) — expected, since XmlSerializer output is driven by [XmlElement]/[XmlAttribute]/[XmlRoot] names, not by the type's assembly.
  • Missing-config path: renamed the scratch App.config away and ran merge from a terminal — "Config file is missing." still printed to that terminal, confirming AppState's static-init ordering (Notifier before Settings, explicit static constructor) survived the move.
  • MCP: a hand-rolled stdio JSON-RPC client (initializetools/listtools/call × 4) against WitcherScriptMerger.exe mcp. tools/list returned exactly 4 tools (scan_conflicts, list_merges, merge_conflicts, get_status) — this specifically confirms the WithToolsFromAssembly(typeof(WsmMcpTools).Assembly) fix (the default parameterless overload only scans the calling assembly; since WsmMcpTools moved to Core, leaving it as-is would have silently registered zero tools while still returning a successful initialize). get_status reported dependenciesValid: true, conflictCount: 1. scan_conflicts correctly reported the test.ws conflict. merge_conflicts (no args) returned {"merged":["test.ws"],"skipped":[]}; list_merges before/after confirmed the new record. All SDK logging landed on stderr, stdout carried clean JSON-RPC only.
  • GUI: launched via Start-Process, confirmed the process stays alive and responsive (MainWindowTitle = Script Merger v0.6.2) rather than crashing on startup. I don't have a way to drive a mouse/keyboard session directly in this environment, so I automated it via Windows UI Automation (System.Windows.Automation from PowerShell) instead — checked the modA/modB tree nodes (via real synthesized mouse clicks — the app's custom SMTree control handles its own mouse events for checkbox toggling and doesn't respond to keyboard/UIA Toggle-pattern automation), clicked "Create Selected Merge" with the output file pre-existing so ConfirmOutputOverwrite would fire. A real native MessageBox.Show titled "Overwrite?" appeared with the expected text, proving the IMergeNotifierMainForm → real MessageBox.Show → real DialogResult → neutral NotifyResult round trip actually works, not just compiles. Clicking "Yes" ran the merge through the stub KDiff3 and a real MergeReportForm ("Merge Finished") appeared, confirming OnMergeReport/InteractiveMergeRunner/Program.MainForm.ShowModal all work together. Clicked its OK button; the app closed cleanly. The merged output file and MergeInventory.xml matched the CLI run's results. (Incidental finding, not a bug: the real machine's own Documents\The Witcher 3\mods.settings file is in a format CustomLoadOrder.Refresh() rejects, which pops a real, unrelated MessageBox.Show warning on startup — additional unplanned confirmation the translation layer works, dismissed as part of the automated run without touching that file.)

What I did not verify

  • The bundle (Categories.BundleText) merge path — unaffected in shape by this PR (same code, same QuickBms/WccLite calls, just moved and now going through AppState), but I didn't round-trip it through a real bundle conflict; this matches the pre-existing verification gap CLAUDE.md already documents for that path.
  • KDiff3's real diff/auto-solve behavior is entirely stubbed out in the E2E tests above by design — KDiff3.cs itself is unmodified except enum-literal swaps, so this isn't new risk, but it also isn't re-verified here.

Code review pass (/code-review --level high)

Ran after the initial implementation, per the task's instructions. It surfaced one real regression, one real (if narrow) safety loss my own description had understated, and several smaller cleanup items — all addressed in a follow-up commit on this branch:

  • Real bug, confirmed empirically: Program lost its only static field read (Notifier/Settings/LoadOrder/Inventory became pass-through properties to AppState), which made it beforefieldinit by default — under which the CLR is free to defer _consoleAttached's field initializer (MaybeAttachConsole()) past Main() entirely, since nothing in Main() necessarily touches a field of Program anymore. I didn't take the reviewer's word for it — built an isolated minimal repro mirroring the exact shape (side-effecting field initializer + pass-through property to a second type + no explicit static constructor) and confirmed: without an explicit static Program() { }, the initializer's side effect never ran at all before Main() read the forwarded property; adding the constructor fixed it every time. In hindsight, this likely explains an unexplained empty-output CLI invocation I saw earlier in this same verification pass and initially attributed to a tooling quirk. Fixed with an explicit static Program() { } plus a comment pointing at the repro so it isn't removed by a future refactor without re-verifying.
  • Real safety regression I'd mischaracterized as cosmetic: LoadOrderValidator's "Custom Load Order Problem" prompt used to explicitly default-focus the "No" button (MessageBoxDefaultButton.Button2); routing it through the neutral IMergeNotifier.ShowMessage(...) (no default-button parameter) silently dropped that, defaulting to "Yes" instead - a real risk (an accidental Enter/Space keypress could now silently rewrite the user's real mods.settings), not the purely-cosmetic loss my first pass described. Fixed properly rather than just re-labeled: added an optional defaultResult parameter to IMergeNotifier.ShowMessage, translated in MainForm to the correct positional MessageBoxDefaultButton for whatever button set is actually shown (ToNativeDefaultButton/ButtonOrder helpers); LoadOrderValidator now passes NotifyResult.No explicitly. Every other existing call site is unaffected (NotifyResult.None = "no preference," WinForms' own default).
  • Real (latent) naming collision: the neutral NotifyIcon enum lived in the same namespace every host file already sees via nested-namespace lookup, and would have silently shadowed System.Windows.Forms.NotifyIcon (the tray-icon class) the moment anything in the host project referenced it — harmless today (nothing does yet), but a confusing landmine for a future "minimize to tray" feature. Renamed to DialogIcon throughout.
  • Confirmed dead code, removed: IMergeNotifier.IsInteractive had zero read call sites anywhere in the codebase, before or after this split.
  • Confirmed redundant, removed: InteractiveMergeRequest.OrderedModNames duplicated OrderedSources[i].Name at its only read site (the pre-loop ConfirmRemainingConflict gate, which always runs before any element of OrderedSources is touched) - the bundle-intermediate-result concern that motivated adding it doesn't actually apply there. Simplified to read OrderedSources[i].Name directly.
  • Acknowledged, not changed: Categories.* fields are now readonly (closes off silent reference-equality breakage now that they're public across the assembly boundary) - a genuine, cheap improvement, made. QuickBms.ExePath/WccLite.ExePath staying publicly mutable and Paths.ValidateDependencyPaths() reading AppState.MergeEngine as a global rather than via DI were both flagged too; I left both as-is with an explanatory comment, since both match a pre-existing pattern already in this codebase (KDiff3.ExePath is legitimately mutated by DependencyForm.cs today; AppState.Notifier/Settings already use the identical global-read pattern) rather than being new anti-patterns introduced by this PR.

All fixes re-verified: dotnet build clean (same 5 pre-existing warnings), dotnet format whitespace --verify-no-changes passes, CLI merge and the full MCP tool round-trip both re-run successfully against the scratch tree after these changes.

Adversarial review pass (multi-agent, against git diff main...pr-5)

The repo owner ran an independent adversarial review of the actual PR diff (separate from the /code-review self-check above) and surfaced 7 findings (4 CONFIRMED, 3 PLAUSIBLE). All 7 addressed in a follow-up commit:

  1. [CONFIRMED, most severe] LoadOrderValidator.cs — destructive Cancel with no warning label. The "Custom Load Order Problem" prompt's Cancel button permanently disables ValidateCustomLoadOrder (writes it to App.config), but after routing through the neutral IMergeNotifier it just read "Cancel" with nothing marking that as destructive — my first-pass description had called the lost MessageBoxManager "Ne&ver" caption "purely cosmetic," which was wrong; the caption was the only thing communicating the button's real semantics. Fixed by spelling out what each button does directly in the message body ("Cancel: NEVER ask again - permanently disables this check."), which doesn't depend on any button-relabeling mechanism working. Side-note carried into CLAUDE.md per the reviewer's flag: MessageBoxManager.Register() hooks via AppDomain.GetCurrentThreadId(), a deprecated API that doesn't reliably return the real Win32 thread ID SetWindowsHookEx needs — the "Never" caption was likely already silently broken before this PR touched anything. That's a pre-existing bug, not one introduced here, but it doesn't change what needed fixing (a Cancel button with genuinely destructive, permanent effect and no textual warning).
  2. [PLAUSIBLE, fixed at the root] Same file — HeadlessMergeNotifier's generic YesNoCancel default is the wrong default for this specific prompt. Its generic "Cancel is safest" guess is actually the destructive choice here, contradicting IMergeNotifier's own documented non-destructive-default contract. Not reachable through this PR's current call graph (LoadOrderValidator.ValidateAndFix is still GUI-only, called only from MainForm.RefreshMergeInventory), but this PR is what made the file headless-callable in the first place by moving it to Core, and my first self-review pass only patched the interactive half (defaultResult was documented as headless-ignored). Fixed at the root: HeadlessMergeNotifier.ShowMessage now returns defaultResult directly when the caller supplies one, instead of always falling through to its per-button-set table — IMergeNotifier's doc comment updated to describe defaultResult as "the caller's own answer for which result is safe for this prompt," honored by both implementations, not just a UI pre-focus hint.
  3. [PLAUSIBLE, fixed] InteractiveMergeRunner.cs — TreeNode extraction moved off the UI thread, but that traded a silent no-op for a possible crash. My initial implementation extracted InteractiveMergeRequests from checked TreeNodes synchronously on the UI thread, before starting the BackgroundWorker (I'd reasoned this was strictly safer than the original's on-worker-thread extraction — it avoided the cross-thread TreeNode access question entirely). The reviewer correctly pointed out the actual risk I'd missed: ExtractRequest casts Tag to ModFileCategory and dereferences node metadata with zero null/type checks. Inside BackgroundWorker.DoWork (where the pre-split code did its equivalent extraction), an exception there is captured into RunWorkerCompletedEventArgs.Error — unread by OnMergeComplete both before and after this PR, so effectively a silent no-op. Run synchronously on the UI thread instead (my change), the identical exception now throws from btnMergeFiles_Click directly, and modern .NET WinForms terminates the process by default for an unhandled UI-thread exception (unlike .NET Framework's more forgiving default). Moved the extraction back inside DoWork, restoring the original threading model and its original (silent-no-op, not ideal, but not new) failure mode.
  4. [CONFIRMED] CLAUDE.md wasn't updated at all, despite documenting a project structure this PR changed completely — still said "Single WinForms project... no MVC/MVP split" and the pre-split folder map, and Units 6-10 of this batch will read it as ground truth. Updated: the Architecture section's opening description and folder map now describe Core vs. host (with a new "Interactive vs. headless split" subsection covering FileMerger/IMergeEngine/InteractiveMergeRunner), the Startup flow section now describes AppState and the Program/AppState split, and the CLI mode section's IMergeNotifier bullet no longer claims the GUI path is "behavior-identical" (see finding 1 above — it isn't, and now says why). Full CLAUDE.md federation is still Unit 10's job; this is scoped to "stop the doc from being actively wrong in the interim," per the reviewer's own framing.
  5. [CONFIRMED] WitcherScriptMerger.Core.csproj didn't carry over <CodeAnalysisRuleSet>DeadCodeDetection.ruleset</CodeAnalysisRuleSet> from the host csproj, so the ~20 files that moved to Core (FileMerger, ModFileIndex, CustomLoadOrder, Paths, the Tools/ wrappers, etc.) silently dropped out of dead-code analysis (CA1801/1804/1811/1812/1823). Added the same ruleset reference (relative path ..\WitcherScriptMerger\DeadCodeDetection.ruleset). Confirmed via rebuild: no new warnings surfaced (nothing in Core is actually dead), so this was a coverage gap, not a sign of undetected dead code.
  6. [CONFIRMED] MergeTextInteractive/MergeTextHeadless had near-identical, fully duplicated bookkeeping (which sources to record into MergeInventory.xml via AddModToMerge, and which to skip because they're the accumulated output file itself or an intermediate bundle-merge byproduct) — a real risk given that bookkeeping feeds a load-bearing hash record (per CLAUDE.md's Compatibility constraints) and a future fix would likely land in only one copy. Factored into one shared RecordMergedSources(merge, source1, source2) helper both methods call.
  7. [PLAUSIBLE, fixed] AppState.MergeEngine defaults to null with no safe fallback, unlike Notifier/Settings (both get real defaults via field initializers) — FileMerger reads it directly with no null check, so any future entry point constructing a FileMerger without first going through Program.Main's startup sequence (a test harness; the Linux CLI/MCP-only host floated for a later unit) would hit an unhandled NullReferenceException deep inside a merge instead of a clear error. Rather than inventing a "safe default" IMergeEngine (Core has no sensible one to offer — there's no merge tool it can fall back to on its own), added an explicit null-check to FileMerger's constructor that throws ArgumentNullException with a message pointing at AppState.MergeEngine/Tools/IMergeEngine.cs. No behavior change for either of the two real call sites (Cli/MergeOperations.RunMerge, InteractiveMergeRunner's constructor) — both already only run after Program.Main has set AppState.MergeEngine.

Re-verified after all seven: dotnet build clean (same 5 pre-existing warnings, and the newly-covered Core project surfaces zero dead-code warnings of its own), dotnet format whitespace --verify-no-changes passes, CLI merge, the full MCP tool round-trip, and a GUI launch-and-stay-responsive smoke check were all re-run successfully against the scratch tree.

Chris Knight and others added 4 commits August 7, 2026 13:11
Move all WinForms-free domain code (FileIndex, Inventory, LoadOrder,
Tools/Hasher+QuickBms+WccLite, Paths, AppSettings, Cli, Mcp) into a new
net10.0 WitcherScriptMerger.Core class library, so it can eventually run
on Linux. The host project keeps Forms/Controls, KDiff3's Win32 P/Invoke,
and Program.cs, referencing Core via ProjectReference.

Key design changes required to make the split compile, beyond plain file
moves - full rationale in the PR description:

- AppState (Core): Notifier/Settings/LoadOrder/Inventory/MergeEngine used
  to be static fields on the host's Program class; moved to a new Core
  class since Core code needs them and can never reference the host
  assembly. Program.cs re-exposes them as pass-through properties so
  host call sites didn't need to change.
- IMergeNotifier neutralized: MessageBoxButtons/MessageBoxIcon/DialogResult
  replaced with NotifyButtons/NotifyIcon/NotifyResult; MainForm translates
  to/from the real WinForms types. ShowModal(Form) dropped from the
  interface entirely - every call site was GUI-only, so the host calls
  MainForm.ShowModal directly instead.
- IMergeEngine (Core) + KDiff3MergeEngine (host): scaffolding so
  FileMerger can move to Core while KDiff3.cs's Win32 P/Invoke stays
  host-only this wave.
- FileMerger split: Core keeps headless + interactive merge orchestration
  (now driven by plain InteractiveMergeRequest data, not TreeNode); the
  host's new InteractiveMergeRunner extracts that data from TreeNodes,
  owns the BackgroundWorker, and shows report forms/plays sounds via
  OnMergeReport/OnPackReport callbacks.
- Extensions.cs split: pure string helpers moved to Core's
  StringExtensions.cs; TreeNode/TreeView WinForms helpers stayed host-side.
- LoadOrderValidator.PromptToPrioritizeMergedMod routed through
  IMergeNotifier instead of a direct MessageBox.Show call (was flagged as
  needing this fix to move into Core at all).

dotnet build WitcherScriptMerger.sln succeeds with the same 5 pre-existing
warnings (all host-side CA1823, unrelated to this change) and zero
warnings/errors from WitcherScriptMerger.Core. dotnet format whitespace
--verify-no-changes passes.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GXAuGMLB44T5Zv5o5ZzKah
- Fix a real regression: Program no longer has an explicit static
  constructor after Notifier/Settings/LoadOrder/Inventory became
  pass-through properties to AppState, so it picked up beforefieldinit -
  under which the CLR is free to defer _consoleAttached's field
  initializer (MaybeAttachConsole()) past Main() entirely, since nothing
  in Main() touches a field of Program anymore. Confirmed empirically
  with an isolated repro (mirrors the exact shape: side-effecting field
  initializer + pass-through property to another type + no static ctor)
  - without an explicit static Program() {}, the initializer's side
    effect never ran before Main() read the forwarded property; with it,
    ordering was correct every time. This likely explains an unexplained
    empty-output CLI invocation earlier in this same session.
- Rename NotifyIcon -> DialogIcon: the original name, declared in the
  root WitcherScriptMerger namespace that every host file already sees
  via nested-namespace lookup, would have silently shadowed
  System.Windows.Forms.NotifyIcon (the tray-icon class) for any
  unqualified reference in host code.
- Add IMergeNotifier.ShowMessage(..., defaultResult) so
  LoadOrderValidator's "Custom Load Order Problem" prompt can still
  request "No" as the pre-focused button, matching the direct
  MessageBox.Show(..., MessageBoxDefaultButton.Button2) call it replaced.
  This was previously mischaracterized as a purely cosmetic loss; it
  wasn't - an accidental Enter/Space keypress could have silently
  rewritten the user's real mods.settings file.
- Remove FileMerger.InteractiveMergeRequest.OrderedModNames: redundant
  with OrderedSources[i].Name at its only read site (the pre-loop
  ConfirmRemainingConflict gate, which always runs before any element of
  OrderedSources is touched).
- Remove IMergeNotifier.IsInteractive: confirmed dead code (zero read
  call sites) both before and after the Core split.
- Make Categories.* fields readonly: closes off silent breakage of the
  reference-equality checks used throughout the codebase
  (`category == Categories.Script`) now that they're public (required
  for cross-assembly access) rather than merely assembly-internal.
- Add code comments acknowledging two accepted, lower-severity
  tradeoffs raised in review: QuickBms/WccLite's ExePath fields staying
  publicly mutable (matches the pre-existing, legitimate
  KDiff3.ExePath-via-DependencyForm mutation pattern) and
  Paths.ValidateDependencyPaths() reading AppState.MergeEngine as a
  global rather than via DI (matches the pre-existing
  AppState.Notifier/Settings pattern this PR extends, not a new
  anti-pattern).

Re-verified after these fixes: dotnet build clean (same 5 pre-existing
warnings), dotnet format --verify-no-changes passes, CLI merge and MCP
tools/list + merge_conflicts round-trip both still succeed against the
scratch tree.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GXAuGMLB44T5Zv5o5ZzKah
1. LoadOrderValidator's YesNoCancel "Custom Load Order Problem" prompt:
   its Cancel button permanently disables ValidateCustomLoadOrder, but
   after the Core split it just reads "Cancel" with nothing marking it
   destructive (the old MessageBoxManager "Ne&ver" relabeling has no
   IMergeNotifier equivalent, and was likely already silently broken
   pre-split regardless - AppDomain.GetCurrentThreadId() doesn't
   reliably return the real Win32 thread ID its SetWindowsHookEx call
   needs). Spelled the Yes/No/Cancel semantics out explicitly in the
   message body instead, where it doesn't depend on any button-caption
   mechanism working.
2. HeadlessMergeNotifier now honors ShowMessage's defaultResult instead
   of always falling through to its generic per-button-set guess -
   that generic guess (Cancel is safest for YesNoCancel) is actually
   the *destructive* choice for LoadOrderValidator's prompt specifically,
   contradicting IMergeNotifier's own non-destructive-default contract.
   This PR is what made LoadOrderValidator reachable from Core (and so,
   in principle, from a headless entry point) in the first place, so
   fixed at the root rather than left contingent on no headless caller
   ever reaching it.
3. InteractiveMergeRunner.MergeByTreeNodesAsync: moved TreeNode
   extraction back inside BackgroundWorker.DoWork instead of running it
   synchronously on the UI thread beforehand, matching the pre-split
   threading model. ExtractRequest dereferences node metadata with no
   null/type check; outside DoWork, an exception there now throws
   synchronously from btnMergeFiles_Click, which modern .NET WinForms
   terminates the process for by default - DoWork instead captures it
   into RunWorkerCompletedEventArgs.Error (still unread by
   OnMergeComplete, same as before this PR), so this restores the
   original "silently does nothing" failure mode rather than a crash.
4. CLAUDE.md: updated the Architecture section (opening description,
   folder map split into Core/host, a new "Interactive vs. headless
   split" subsection) and corrected the IMergeNotifier section's now-
   false claim that MainForm's translation is behavior-identical to the
   old direct calls (see fix 1). Full CLAUDE.md federation is a later
   unit's job - this just stops the doc from actively lying to it and
   to the other Wave-0 units reading it as ground truth in the meantime.
5. WitcherScriptMerger.Core.csproj now references the same
   DeadCodeDetection.ruleset the host project uses, so the ~20 files
   that moved to Core don't silently drop out of dead-code analysis.
6. Factored MergeTextInteractive/MergeTextHeadless's identical
   AddModToMerge bookkeeping (which sources to record, and which to
   skip because they're the output file itself or an intermediate
   bundle-merge byproduct) into one shared RecordMergedSources helper -
   this bookkeeping feeds MergeInventory.xml's hashes, which are
   load-bearing, so a future fix now has one call site, not two that
   can silently drift apart.
7. FileMerger's constructor now throws a clear ArgumentNullException if
   given a null IMergeEngine, instead of leaving AppState.MergeEngine's
   null default (unlike its siblings Notifier/Settings, which both get
   real defaults) to eventually surface as an unhandled
   NullReferenceException deep inside Merge()/MergeHeadless() the first
   time some future entry point (a test harness, the Linux CLI/MCP-only
   host planned for a later unit) constructs a FileMerger without going
   through Program.Main's startup sequence first.

Re-verified after all seven: dotnet build clean (same 5 pre-existing
warnings), dotnet format --verify-no-changes passes, CLI merge and the
full MCP tool round-trip both still succeed against the scratch tree,
GUI still launches and stays responsive.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01GXAuGMLB44T5Zv5o5ZzKah
…oject

# Conflicts:
#	WitcherScriptMerger.Core/LoadOrder/LoadOrderValidator.cs
#	WitcherScriptMerger/Forms/MainForm.cs
#	WitcherScriptMerger/HeadlessMergeNotifier.cs
#	WitcherScriptMerger/IMergeNotifier.cs
@TheValiantOne
TheValiantOne merged commit 2f3f1ee into main Aug 7, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant