Split into WitcherScriptMerger.Core + host GUI/CLI/MCP project - #5
Merged
Conversation
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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Splits the single WinForms project into a cross-platform
WitcherScriptMerger.Coreclass library (net10.0, no WinForms reference) plus the existing GUI/CLI/MCP host project (net10.0-windows7.0, unchangedWinExe/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/(includingFileMerger, 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 mentionProgram.Notifier/Program.Settings/Program.LoadOrder/Program.Inventorywere static fields on the host'sProgramclass. 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 onProgramas originally declared.Fix: a new
WitcherScriptMerger.AppStatestatic class in Core now ownsNotifier/Settings/LoadOrder/Inventory, plus a newMergeEngine(see below).Program.cs'sNotifier/Settings/LoadOrder/Inventorybecame pass-through properties forwarding toAppState, so every existing host call site (Program.Notifier.ShowError(...),Program.Inventory = ..., etc.) needed zero changes — only files that moved into Core needed the mechanicalProgram.X→AppState.Xrename.AppStatehas an explicit (empty) static constructor to suppressbeforefieldinit, so its field-initializer ordering (NotifierbeforeSettings, matching the originalProgram.csdeclaration order —AppSettings's constructor callsNotifier.ShowErroron a missing config file) stays deterministic, andMaybeAttachConsole()still runs before anything can report a startup error. Verified empirically: renamingApp.configaway and runningmergefrom 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 callFile.Exists(KDiff3.ExePath)directly, butKDiff3.csstays host-only. Fixed by addingIMergeEngine.ValidateExePath()(implemented byKDiff3MergeEngineasFile.Exists(KDiff3.ExePath)) soPaths(Core) validates the merge engine's dependency through the sameAppState.MergeEngineseam instead of reaching into Tools/KDiff3.cs directly.IMergeNotifierneutralizationNotifyResult/NotifyButtons/DialogIconin Core (NotifyTypes.cs), 1:1 with theDialogResult/MessageBoxButtons/MessageBoxIconmembers actually in use (buttons: the full 6-value setHeadlessMergeNotifieralready 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. SinceFormhas no cross-platform equivalent and nothing outside the host actually needs this method, it stays as a plain (non-interface) public method onMainForm, and the interactive-only host code (InteractiveMergeRunner) callsProgram.MainForm.ShowModal(...)directly instead of going through the notifier abstraction.HeadlessMergeNotifier's old defensiveShowModalstub (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/ShowErrornow implement the neutral interface and translate to/from realMessageBox.Show(...)/DialogResultviaToNative(NotifyButtons)/ToNative(DialogIcon)/ToNeutral(DialogResult)helpers.LoadOrderValidator.PromptToPrioritizeMergedModused a directMessageBox.Show(...)with aMessageBoxManager-customized "Ne&ver" Cancel-button caption andMessageBoxDefaultButton.Button2. Per the task brief's explicit instruction, this now routes throughIMergeNotifier.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 realmods.settings), so I added it back properly: see "Code review pass" below for thedefaultResultparameter that preserves it. Note for the reviewer: a parallel Wave-0 unit may also be touching this exact file (aLoadOrderValidatorfix) — expect a possible merge conflict there.IMergeEngine— scaffolding, not a permanent abstractionTwo methods rather than a single method with an
interactivebool flag:Merge(...)(mirrorsKDiff3.Run, may block on the tool's own UI) andMergeHeadless(...)(mirrorsKDiff3.RunHeadless, never blocks, can reportNeedsManualResolution). 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.csitself is unchanged except three enum-literal swaps (MessageBoxButtons/MessageBoxIcon→NotifyButtons/DialogIcon) and dropping its now-unneededusing 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.FileMergersplit shapeCore's
FileMergerkeeps: theMergeSource/HeadlessMergeSummarytypes (unchanged), the already-headless methods (MergeConflictsHeadlesset al., now callingMergeEngine.MergeHeadlessinstead ofKDiff3.RunHeadlessdirectly), shared helpers (ConfirmOutputOverwrite,GetUnpackedFiles,UnpackFile,PackNewBundle, cleanup), and new interactive orchestration methods (MergeFilesInteractive,MergeFlatFileInteractive,MergeBundleFileInteractive,MergeTextInteractive,ConfirmRemainingConflict,ConfirmContinueAfterCanceledMerge) that mirror the oldMergeByTreeNodesAsync/MergeFlatFileNode/MergeBundleFileNode/MergeText/HandleCanceledMergealmost line-for-line, but driven by a newInteractiveMergeRequest(relative path, bundle flag, vanilla file path, orderedMergeSource[]) instead ofTreeNode[].I considered instead having the host adapter convert TreeNode selections into the same shape
MergeConflictsHeadlessalready 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/nowMergeTextInteractivecalls the report callback once per successful merge in a multi-mod chain, not once per file), theConfirmRemainingConflictload-order gate (interactive-only), andConfirmContinueAfterCanceledMerge'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 readsOrderedSources[i].Name(derived viaModFile.GetModNameFromPath, which falls back toPaths.MergedBundleContentfor a source that's an intermediate bundle-merge result rather than an original per-mod file). I initially added a separate, explicitly-carriedOrderedModNamesstring 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 ofOrderedSourcesis 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.SystemSoundsnor anyForms.*type appears in Core'sFileMerger— both moved to the host's newInteractiveMergeRunner(WitcherScriptMerger/Inventory/InteractiveMergeRunner.cs), which:BackgroundWorker(Core'sFileMergerno longer has one).InteractiveMergeRequest[]from checkedTreeNodes (the one and only placeTreeNode.GetMetadata()is called for this flow).MergeByTreeNodesAsync/RepackBundleAsyncwith the same signatures the oldFileMergerhad, soMainForm.cs's call sites only needednew FileMerger(...)→new InteractiveMergeRunner(...)— nothing else inMainForm.cschanged for this.OnMergeReport/OnPackReportcallbacks that constructMergeReportForm/PackReportForm, play the completion sound (PlayCompletionSoundssetting), and callProgram.MainForm.ShowModal(...)— i.e., exactly what the old inlineusing (var reportForm = ...) { ShowModal }blocks did, just relocated.if (_bgWorker.IsBusy) throwguard from the originalRepackBundleAsync.Deliberate minor deviation: the shared
ShowPackReportcallback setsProgressInfo.CurrentAction = "Showing pack report"before showingPackReportForm. The originalMergeByTreeNodesAsync's inline pack-report block did this; the originalRepackBundleAsync'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 WinFormsTreeNode/TreeViewP/Invoke helpers that must stay host-side. Split into Core'sStringExtensions.cs(sameWitcherScriptMergernamespace, 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'sExtensions.cs(WinForms-only content,System.Text.RegularExpressions/using System;-only-where-still-needed trimmed).Verification
dotnet build WitcherScriptMerger.sln: succeeds.WitcherScriptMerger.Core.csprojhas zero references toSystem.Windows.Formsanywhere (confirmed by grep — the only hits are in comments explaining why there aren't any) and, since Core's TFM is barenet10.0with noUseWindowsForms, any such reference would fail to compile at all, not just warn.WitcherScriptMerger.csprojbuilds with the same 5 pre-existingCA1823warnings that exist onmaintoday (MessageBoxManager.cs,Program.cs,SMTree.cs— all unrelated to this change), zero new warnings.dotnet format whitespace WitcherScriptMerger.sln --verify-no-changespasses.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 sharedtest.wsscript differently — a guaranteed conflict) with placeholderquickbms.exe/witcher3.bms/wcc_lite.exefiles (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 stubKDiff3.exe(adotnet 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, unmodifiedTools/KDiff3.cscode path end to end —Process.Start,EnsureUtf16Encoding,BuildArgs, exit-code handling, and forRunHeadlessthe window-detection loop (no window ever appears, so it reportsAutoSolvedalmost immediately) — the only thing not real is KDiff3's own diff algorithm, which this PR does not touch.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-destructiveHeadlessMergeNotifierdefault for the resulting "Overwrite?" prompt) — both are the documented existing behavior, now running throughAppState/IMergeEngine.MergeInventory.xml's schema is unchanged (<MergeInventory><Merge><RelativePath>.../<IncludedMod Hash="...">name</IncludedMod>.../<MergedModName>...) — expected, sinceXmlSerializeroutput is driven by[XmlElement]/[XmlAttribute]/[XmlRoot]names, not by the type's assembly.App.configaway and ranmergefrom a terminal — "Config file is missing." still printed to that terminal, confirmingAppState's static-init ordering (Notifier before Settings, explicit static constructor) survived the move.initialize→tools/list→tools/call× 4) againstWitcherScriptMerger.exe mcp.tools/listreturned exactly 4 tools (scan_conflicts,list_merges,merge_conflicts,get_status) — this specifically confirms theWithToolsFromAssembly(typeof(WsmMcpTools).Assembly)fix (the default parameterless overload only scans the calling assembly; sinceWsmMcpToolsmoved to Core, leaving it as-is would have silently registered zero tools while still returning a successfulinitialize).get_statusreporteddependenciesValid: true,conflictCount: 1.scan_conflictscorrectly reported thetest.wsconflict.merge_conflicts(no args) returned{"merged":["test.ws"],"skipped":[]};list_mergesbefore/after confirmed the new record. All SDK logging landed on stderr, stdout carried clean JSON-RPC only.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.Automationfrom PowerShell) instead — checked themodA/modBtree nodes (via real synthesized mouse clicks — the app's customSMTreecontrol handles its own mouse events for checkbox toggling and doesn't respond to keyboard/UIAToggle-pattern automation), clicked "Create Selected Merge" with the output file pre-existing soConfirmOutputOverwritewould fire. A real nativeMessageBox.Showtitled "Overwrite?" appeared with the expected text, proving theIMergeNotifier→MainForm→ realMessageBox.Show→ realDialogResult→ neutralNotifyResultround trip actually works, not just compiles. Clicking "Yes" ran the merge through the stub KDiff3 and a realMergeReportForm("Merge Finished") appeared, confirmingOnMergeReport/InteractiveMergeRunner/Program.MainForm.ShowModalall work together. Clicked its OK button; the app closed cleanly. The merged output file andMergeInventory.xmlmatched the CLI run's results. (Incidental finding, not a bug: the real machine's ownDocuments\The Witcher 3\mods.settingsfile is in a formatCustomLoadOrder.Refresh()rejects, which pops a real, unrelatedMessageBox.Showwarning 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
Categories.BundleText) merge path — unaffected in shape by this PR (same code, sameQuickBms/WccLitecalls, just moved and now going throughAppState), 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.csitself 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:
Programlost its only static field read (Notifier/Settings/LoadOrder/Inventorybecame pass-through properties toAppState), which made itbeforefieldinitby default — under which the CLR is free to defer_consoleAttached's field initializer (MaybeAttachConsole()) pastMain()entirely, since nothing inMain()necessarily touches a field ofProgramanymore. 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 explicitstatic Program() { }, the initializer's side effect never ran at all beforeMain()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 explicitstatic Program() { }plus a comment pointing at the repro so it isn't removed by a future refactor without re-verifying.LoadOrderValidator's "Custom Load Order Problem" prompt used to explicitly default-focus the "No" button (MessageBoxDefaultButton.Button2); routing it through the neutralIMergeNotifier.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 realmods.settings), not the purely-cosmetic loss my first pass described. Fixed properly rather than just re-labeled: added an optionaldefaultResultparameter toIMergeNotifier.ShowMessage, translated inMainFormto the correct positionalMessageBoxDefaultButtonfor whatever button set is actually shown (ToNativeDefaultButton/ButtonOrderhelpers);LoadOrderValidatornow passesNotifyResult.Noexplicitly. Every other existing call site is unaffected (NotifyResult.None= "no preference," WinForms' own default).NotifyIconenum lived in the same namespace every host file already sees via nested-namespace lookup, and would have silently shadowedSystem.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 toDialogIconthroughout.IMergeNotifier.IsInteractivehad zero read call sites anywhere in the codebase, before or after this split.InteractiveMergeRequest.OrderedModNamesduplicatedOrderedSources[i].Nameat its only read site (the pre-loopConfirmRemainingConflictgate, which always runs before any element ofOrderedSourcesis touched) - the bundle-intermediate-result concern that motivated adding it doesn't actually apply there. Simplified to readOrderedSources[i].Namedirectly.Categories.*fields are nowreadonly(closes off silent reference-equality breakage now that they'republicacross the assembly boundary) - a genuine, cheap improvement, made.QuickBms.ExePath/WccLite.ExePathstaying publicly mutable andPaths.ValidateDependencyPaths()readingAppState.MergeEngineas 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.ExePathis legitimately mutated byDependencyForm.cstoday;AppState.Notifier/Settingsalready use the identical global-read pattern) rather than being new anti-patterns introduced by this PR.All fixes re-verified:
dotnet buildclean (same 5 pre-existing warnings),dotnet format whitespace --verify-no-changespasses, 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-reviewself-check above) and surfaced 7 findings (4 CONFIRMED, 3 PLAUSIBLE). All 7 addressed in a follow-up commit:LoadOrderValidator.cs— destructive Cancel with no warning label. The "Custom Load Order Problem" prompt's Cancel button permanently disablesValidateCustomLoadOrder(writes it toApp.config), but after routing through the neutralIMergeNotifierit just read "Cancel" with nothing marking that as destructive — my first-pass description had called the lostMessageBoxManager"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 intoCLAUDE.mdper the reviewer's flag:MessageBoxManager.Register()hooks viaAppDomain.GetCurrentThreadId(), a deprecated API that doesn't reliably return the real Win32 thread IDSetWindowsHookExneeds — 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).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, contradictingIMergeNotifier's own documented non-destructive-default contract. Not reachable through this PR's current call graph (LoadOrderValidator.ValidateAndFixis still GUI-only, called only fromMainForm.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 (defaultResultwas documented as headless-ignored). Fixed at the root:HeadlessMergeNotifier.ShowMessagenow returnsdefaultResultdirectly when the caller supplies one, instead of always falling through to its per-button-set table —IMergeNotifier's doc comment updated to describedefaultResultas "the caller's own answer for which result is safe for this prompt," honored by both implementations, not just a UI pre-focus hint.InteractiveMergeRunner.cs— TreeNode extraction moved off the UI thread, but that traded a silent no-op for a possible crash. My initial implementation extractedInteractiveMergeRequests from checkedTreeNodes synchronously on the UI thread, before starting theBackgroundWorker(I'd reasoned this was strictly safer than the original's on-worker-thread extraction — it avoided the cross-threadTreeNodeaccess question entirely). The reviewer correctly pointed out the actual risk I'd missed:ExtractRequestcastsTagtoModFileCategoryand dereferences node metadata with zero null/type checks. InsideBackgroundWorker.DoWork(where the pre-split code did its equivalent extraction), an exception there is captured intoRunWorkerCompletedEventArgs.Error— unread byOnMergeCompleteboth 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 frombtnMergeFiles_Clickdirectly, 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 insideDoWork, restoring the original threading model and its original (silent-no-op, not ideal, but not new) failure mode.CLAUDE.mdwasn'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 coveringFileMerger/IMergeEngine/InteractiveMergeRunner), the Startup flow section now describesAppStateand theProgram/AppStatesplit, and the CLI mode section'sIMergeNotifierbullet 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.WitcherScriptMerger.Core.csprojdidn't carry over<CodeAnalysisRuleSet>DeadCodeDetection.ruleset</CodeAnalysisRuleSet>from the host csproj, so the ~20 files that moved to Core (FileMerger, ModFileIndex, CustomLoadOrder, Paths, theTools/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.MergeTextInteractive/MergeTextHeadlesshad near-identical, fully duplicated bookkeeping (which sources to record intoMergeInventory.xmlviaAddModToMerge, 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 (perCLAUDE.md's Compatibility constraints) and a future fix would likely land in only one copy. Factored into one sharedRecordMergedSources(merge, source1, source2)helper both methods call.AppState.MergeEnginedefaults tonullwith no safe fallback, unlikeNotifier/Settings(both get real defaults via field initializers) —FileMergerreads it directly with no null check, so any future entry point constructing aFileMergerwithout first going throughProgram.Main's startup sequence (a test harness; the Linux CLI/MCP-only host floated for a later unit) would hit an unhandledNullReferenceExceptiondeep 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 toFileMerger's constructor that throwsArgumentNullExceptionwith a message pointing atAppState.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 afterProgram.Mainhas setAppState.MergeEngine.Re-verified after all seven:
dotnet buildclean (same 5 pre-existing warnings, and the newly-covered Core project surfaces zero dead-code warnings of its own),dotnet format whitespace --verify-no-changespasses, 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.