Route LoadOrderValidator's prompt through Program.Notifier - #1
Merged
Merged
Conversation
PromptToPrioritizeMergedMod called MessageBox.Show directly instead of Program.Notifier.ShowMessage, unlike every other domain call site. Its sole caller today is invoked only from MainForm.cs, so it was harmless in practice, but it was a landmine for any future headless (CLI/MCP) load-order validation path, which would otherwise hit an unmediated WinForms MessageBox.Show with no message pump watching it. IMergeNotifier.ShowMessage gained a trailing optional MessageBoxDefaultButton parameter (default Button1, matching MessageBox.Show's own default) so the prompt's Button2 (No) default survives the move - dropping it silently would have flipped the default action from "leave load order alone" to "rewrite mods.settings". MainForm.ShowMessage forwards it to the 6-arg MessageBox.Show overload; HeadlessMergeNotifier ignores it. The MessageBoxManager relabeling of the Cancel button to "Ne&ver" is removed rather than preserved: it depends on a SetWindowsHookEx hook registered on the calling thread, but Program.Notifier.ShowMessage (MainForm.ShowMessage) marshals the actual MessageBox.Show call onto the UI thread via Invoke when called off-thread - as this call always is, via MainForm's Task.Run - so the hook would never see the dialog's window messages once routed through the notifier. Kept as dead code it would look functional without being so. The Cancel button now reads "Cancel" instead of "Never"; the DialogResult value and its handling in ValidateAndFix are unchanged. Also guarded ValidateAndFix's Cancel branch on Program.Notifier.IsInteractive: HeadlessMergeNotifier's fixed non-destructive default for YesNoCancel is Cancel, which previously mapped to "Never" here and would have silently persisted ValidateCustomLoadOrder=false to App.config on any future headless run that reaches this code path - exactly the landmine this change exists to defuse. Verified: Program.Notifier is reassigned to MainForm in Program.cs before Application.Run, and the only path that reaches PromptToPrioritizeMergedMod (MainForm_Shown -> RefreshMergeInventory -> LoadOrderValidator.ValidateAndFix) runs after Shown, never from MainForm's constructor - so this change doesn't introduce a window where the prompt silently goes to a HeadlessMergeNotifier instead of the GUI. No GUI automation harness is available in this environment; verified by dotnet build (no new warnings), dotnet format whitespace --verify-no-changes, and code inspection confirming button set, icon, message text, and DialogResult handling are unchanged from the original MessageBox.Show call. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GXAuGMLB44T5Zv5o5ZzKah
- LoadOrderValidator.cs: simplify back to unqualified MessageBoxButtons/ MessageBoxIcon/MessageBoxDefaultButton now that the fully-qualified form (matched literally to the task's example) reads as inconsistent next to the same file's own unqualified DialogResult usage and the rest of the codebase's convention wherever `using System.Windows.Forms;` is already present. - LoadOrderValidator.cs: expand the comment on the dropped "Ne&ver" relabel. Review correctly pointed out the old MessageBoxManager hook genuinely worked before this change (MessageBox.Show ran directly on the same background thread Register() hooked, no Invoke involved) - this is a real, disclosed regression in how the Cancel button reads, not a no-op cleanup, and the comment now says so plainly along with why it can't be preserved through Program.Notifier without extending IMergeNotifier with custom button-text support (out of scope here). - HeadlessMergeNotifier.cs: comment on why defaultButton is accepted but not consulted when choosing the headless DialogResult, so it doesn't read as an oversight to a future caller relying on it. Not addressed here, flagged for other units instead: HeadlessMergeNotifier .Write already routes MessageBoxIcon.None messages to stdout, which is a pre-existing MCP stdout-hygiene risk unrelated to this change (belongs to the MCP-hardening unit); MainForm.cs's PromptToDeleteForChangedHash has an analogous still-"Ne&ver"-labeled prompt that now reads inconsistently with this one, but that method is GUI-layer code outside LoadOrderValidator.cs's scope. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01GXAuGMLB44T5Zv5o5ZzKah
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
LoadOrderValidator.PromptToPrioritizeMergedModcalledMessageBox.Showdirectly instead ofProgram.Notifier.ShowMessage, unlike every other domain call site in the codebase. Its sole caller (ValidateAndFix) is currently only ever invoked fromForms/MainForm.cs, so this was harmless today, but it's a landmine for any future headless (CLI/MCP) load-order validation path — an unmediated WinFormsMessageBox.Showwith no message pump watching it. This change routes it throughProgram.Notifier.ShowMessage, matching the pattern used everywhere else (e.g.LoadOrder/CustomLoadOrder.cs'sShowWarningForMalformedFile).IMergeNotifier.ShowMessagegained a trailing optionalMessageBoxDefaultButton defaultButton = MessageBoxDefaultButton.Button1parameter. The original call passedMessageBoxDefaultButton.Button2(defaulting focus to "No"); dropping it silently would have flipped the Enter-key default from "leave my load order alone" to "rewrite mods.settings" — not behavior-preserving.MainForm.ShowMessageforwards it to the 6-argMessageBox.Showoverload;HeadlessMergeNotifierignores it (no dialog is ever shown headlessly — see below). Only two types implementIMergeNotifier(MainForm,HeadlessMergeNotifier), both updated; the new parameter is a trailing optional so no existing call site needed changes.MessageBoxManager.Register()/Cancel = "Ne&ver"/Unregister()wrapping (aSetWindowsHookEx-based hack to relabel the Cancel button "Never") is removed, not preserved, and is a genuine, disclosed regression — not a no-op cleanup. The old hook worked because the oldMessageBox.Showcall ran directly on the same background threadRegister()hooked (no owner window, no marshalling).Program.Notifier.ShowMessage→MainForm.ShowMessagemarshals the actualMessageBox.Showcall onto the UI thread viaInvokewhenever called off-thread — which this call always is, sinceLoadOrderValidator.ValidateAndFixruns insideMainForm'sTask.Run. The hook (registered on the calling thread) can no longer see the dialog's window messages once routed through the notifier. Preserving it would require adding custom button-text support toIMergeNotifier, which is out of scope for this fix. User-visible effect: the Cancel button now reads "Cancel" instead of "Never" — clicking it still permanently disables this validation check (unchangedDialogResultsemantics), just without a label saying so.ValidateAndFix's Cancel branch is now additionally guarded onProgram.Notifier.IsInteractive.HeadlessMergeNotifier's fixed non-destructive default forYesNoCancelisCancel, which at this specific call site means "Never" →Settings.Set("ValidateCustomLoadOrder", false); Settings.Save(). Without the guard, a future headless caller reaching this code would silently persist a settings change toApp.config— exactly the landmine this PR exists to defuse. With the guard, a headless run is a safe no-op instead.Why
Closes a "safe only by accident" gap flagged for follow-on headless load-order validation work:
LoadOrderValidatorwas the one remaining domain-layer file bypassing theIMergeNotifierabstraction that the rest of the codebase (CustomLoadOrder,FileMerger,AppSettings,Paths, theTools/*wrappers) already routes through, which is what makes CLI/MCP mode possible for everything else.Verified
dotnet build WitcherScriptMerger.slnsucceeds with no new warnings (same 7 pre-existing warnings asmain:NU1510, andCA1823unused-field warnings in unrelated files).dotnet format whitespace WitcherScriptMerger.sln --verify-no-changespasses (exit 0).YesNoCancel), icon (Exclamation), message text, andDialogResulthandling inValidateAndFixare byte-for-byte identical to the originalMessageBox.Showcall (the "Ne&ver" label change is the one disclosed exception, see above).Program.csthatProgram.Notifieris reassigned from the defaultHeadlessMergeNotifiertoMainFormimmediately after construction, beforeApplication.Runstarts the message loop — and confirmed by tracing callers thatPromptToPrioritizeMergedMod's only reachable path (MainForm_Shown→RefreshMergeInventory→LoadOrderValidator.ValidateAndFix) runs from theShownevent and later user-triggered handlers, never fromMainForm's constructor. So this change can't introduce a window where the prompt silently goes to the headless notifier instead of showing a dialog to the interactive user.MessageBox.Showcalls: the only ones left are inForms/MainForm.csandForms/DependencyForm.cs, both GUI-layer code where a direct call is correct (noProgram.Notifierindirection needed there).code-reviewskill against this branch. It surfaced 7 findings; addressed the 3 in scope (see commit "Address code-review findings..."): simplified enum literals back to unqualified for in-file consistency, strengthened the "Ne&ver" regression comment after review correctly identified it as a real behavior change rather than the inert cleanup an earlier, terser comment could have read as, and documented whyHeadlessMergeNotifieraccepts but ignoresdefaultButton. The other 4 findings are out of scope for this unit and are noted below for whoever picks up the relevant follow-on work.Out of scope, flagged for other units
HeadlessMergeNotifier.WriteroutesMessageBoxIcon.Nonemessages to stdout, which risks corrupting the MCP JSON-RPC stream if reached during an MCP session (pre-existing behavior, not introduced here, but live in a file this PR touches) — belongs to MCP-hardening work.Forms/MainForm.cs'sPromptToDeleteForChangedHash(an analogous "permanently disable a check" prompt forValidateMergeSources) still uses theMessageBoxManager"Ne&ver" relabel and still works there (same-thread, noProgram.Notifierinvolved) — after this PR, the app has one prompt that says "Never" and one that says "Cancel" for structurally similar choices.PromptToDeleteForChangedHashis GUI-layer code outsideLoadOrderValidator.cs, so not touched here.Heads-up for other in-flight units
This PR touches
IMergeNotifier.cs,HeadlessMergeNotifier.cs, andMainForm.cs'sShowMessage— the interface a later unit is expected to refactor toward a UI-neutral return type. The addedMessageBoxDefaultButtonparameter is a trailing optional and can't break any existing caller, but whoever picks up that refactor will hit a textual merge conflict in these three files.Disclosure
This PR was substantially produced with Claude Code (an AI coding agent), per this repo's AI-assisted-development policy in
CONTRIBUTING.md. I've reviewed the diff and can explain any part of it if asked.