From 992aa711342db6724131ff19b735cb20b5b051a6 Mon Sep 17 00:00:00 2001 From: Chris Knight Date: Sun, 9 Aug 2026 07:52:01 -0400 Subject: [PATCH] Add WSM_ environment-variable override for AppSettings Lets a caller like a Vortex extension point WSM at a game/mods directory (WSM_GameDirectory, WSM_ModsDirectory, etc.) without hand-editing WitcherScriptMerger.exe.config on disk - the fragile, lock-free pattern Vortex's existing, unrelated-fork integration uses today. Get/Get check Environment.GetEnvironmentVariable("WSM_" + key) first, generically for any key, before falling through to the existing ConfigurationManager-backed lookup; Set/Save are untouched and still only ever write to App.config. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01GXAuGMLB44T5Zv5o5ZzKah --- WitcherScriptMerger.Core/AppSettings.cs | 65 ++++++-- WitcherScriptMerger.Tests/AppSettingsTests.cs | 153 ++++++++++++++++++ 2 files changed, 204 insertions(+), 14 deletions(-) create mode 100644 WitcherScriptMerger.Tests/AppSettingsTests.cs diff --git a/WitcherScriptMerger.Core/AppSettings.cs b/WitcherScriptMerger.Core/AppSettings.cs index 2c6fcb5..97fab67 100644 --- a/WitcherScriptMerger.Core/AppSettings.cs +++ b/WitcherScriptMerger.Core/AppSettings.cs @@ -32,6 +32,50 @@ public AppSettings() } } + // Prefix applied to a setting's key to form the environment-variable name that + // overrides it (e.g. the "GameDirectory" setting is overridden by "WSM_GameDirectory"). + // Generic by construction - covers every current key and any added + // later with zero per-key code changes, since it's just string concatenation, not + // an enumerated switch. + public const string EnvironmentVariablePrefix = "WSM_"; + + // Returns the environment-variable override for a settings key, or null if none is + // set. Static and side-effect-free (no CachedConfig/AppState touch at all) so it's + // safe to unit-test without constructing a live AppSettings instance - see + // WitcherScriptMerger.Tests/CLAUDE.md's "AppState.Settings-safety constraints" for + // why constructing one outside a real GUI/CLI/MCP entry point is unsafe (its + // constructor calls Environment.Exit(1) if it can't find a config file, which kills + // the whole dotnet test process rather than just failing one test). + public static string GetEnvironmentOverride(string key) + { + return Environment.GetEnvironmentVariable(EnvironmentVariablePrefix + key); + } + + // Resolves a key's raw string value: an environment-variable override first, then + // falling through to the existing ConfigurationManager-backed lookup. Both Get and + // Get route through this single place so an env-var-sourced value goes through + // the exact same downstream handling (Get's Parse-based conversion in particular) + // as one read from App.config - never a separate ad-hoc parser. + string GetRawValue(string key) + { + var envValue = GetEnvironmentOverride(key); + if (envValue != null) + return envValue; + + if (CachedConfig.HasFile) + return CachedConfig.AppSettings.Settings[key].Value; + + AppState.Notifier.ShowError($"Config file doesn't exist:\n\n{CachedConfig.FilePath}"); + return null; + } + + // Deliberately unaware of GetEnvironmentOverride: this still only ever writes to + // CachedConfig/App.config, same as before the env-var override existed. If a + // WSM_ override is active for a key this call targets, Get/Get keep + // returning the override afterward regardless of what's written and Save()d here + // - a known, accepted asymmetry (an env var is meant to act as a caller-supplied + // override of whatever App.config/the GUI would otherwise produce), not a bug to + // paper over in this method. public void Set(string key, object value) { try @@ -48,16 +92,13 @@ public T Get(string key) { try { - if (CachedConfig.HasFile) - { - var valueString = CachedConfig.AppSettings.Settings[key].Value; - var parseMethod = typeof(T).GetMethod("Parse", new Type[] { typeof(string) }); - var valueObject = parseMethod.Invoke(null, new object[] { valueString }); - return (T)valueObject; - } + var valueString = GetRawValue(key); + if (valueString == null) + return default(T); - AppState.Notifier.ShowError($"Config file doesn't exist:\n\n{CachedConfig.FilePath}"); - return default(T); + var parseMethod = typeof(T).GetMethod("Parse", new Type[] { typeof(string) }); + var valueObject = parseMethod.Invoke(null, new object[] { valueString }); + return (T)valueObject; } catch { @@ -69,11 +110,7 @@ public string Get(string key) { try { - if (CachedConfig.HasFile) - return CachedConfig.AppSettings.Settings[key].Value; - - AppState.Notifier.ShowError($"Config file doesn't exist:\n\n{CachedConfig.FilePath}"); - return string.Empty; + return GetRawValue(key) ?? string.Empty; } catch { diff --git a/WitcherScriptMerger.Tests/AppSettingsTests.cs b/WitcherScriptMerger.Tests/AppSettingsTests.cs new file mode 100644 index 0000000..3d67fb2 --- /dev/null +++ b/WitcherScriptMerger.Tests/AppSettingsTests.cs @@ -0,0 +1,153 @@ +using System; +using System.Runtime.CompilerServices; +using Xunit; + +namespace WitcherScriptMerger.Tests +{ + // Coverage for AppSettings' WSM_ environment-variable override, which Get/Get + // now check ahead of App.config's block (see AppSettings.cs and Core's + // CLAUDE.md's "Settings & persistence" section). This exists so a caller like a Vortex + // extension can point WSM at a game/mods directory without hand-editing + // WitcherScriptMerger.exe.config - unlike that approach, an env var has no lock/mutex + // contention with a concurrently-running WSM process, since nothing is written to disk. + // + // None of these tests call `new AppSettings()` directly. AppSettings' constructor calls + // Environment.Exit(1) if it can't find a config file next to + // Assembly.GetEntryAssembly().Location, and under dotnet test's testhost.dll host (no + // matching .config) that kills the entire test process, not just one test - see + // WitcherScriptMerger.Tests/CLAUDE.md's "AppState.Settings-safety constraints". + // + // GetEnvironmentOverride is exercised directly (it's static and side-effect-free - no + // CachedConfig/AppState touch at all). Get/Get are exercised too, but only via + // RuntimeHelpers.GetUninitializedObject, which skips the constructor entirely; this is + // safe specifically because, per GetRawValue's short-circuit return, an env override + // being present means CachedConfig (and therefore AppState.Notifier) is never touched - + // confirmed by reading GetRawValue itself, not assumed. This also verifies Get's + // Parse-based type conversion runs identically for an env-sourced value as it would for + // one read from App.config, since GetRawValue is the one place both sources funnel + // through before Get ever sees the string. + // + // The env-var-wins-over-an-actual-config-file case (the other half of "precedence") and + // the full CLI merge pipeline picking up WSM_GameDirectory/WSM_ModsDirectory are instead + // verified end-to-end via a scratch game/mods tree with no App.config edits at all - see + // this feature's PR description for that run, per WitcherScriptMerger.Tests/CLAUDE.md's + // "AppState.Settings-safety constraints" guidance to prefer isolated logic tests here + // over risking the test process. + public class AppSettingsTests + { + [Fact] + public void GetEnvironmentOverride_NotSet_ReturnsNull() + { + Assert.Null(AppSettings.GetEnvironmentOverride(UniqueKey())); + } + + [Theory] + [InlineData("GameDirectory")] + [InlineData("ModsDirectory")] + [InlineData("MergedModName")] + [InlineData("QuickBmsPath")] + [InlineData("QuickBmsPluginPath")] + [InlineData("WccLitePath")] + [InlineData("CheckBundleContents")] + [InlineData("SomeHypotheticalFutureKey")] + public void GetEnvironmentOverride_WorksForAnyKey_NoPerKeyCodeNeeded(string key) + { + // Covers every real key today plus one that appears nowhere in + // App.config, confirming the prefix-and-lookup is genuinely generic string + // concatenation rather than a hardcoded enumerated list somewhere. + WithEnvironmentVariable(key, "override-for-" + key, () => + { + Assert.Equal("override-for-" + key, AppSettings.GetEnvironmentOverride(key)); + }); + } + + [Fact] + public void Get_WithEnvironmentOverride_ReturnsOverrideValue() + { + var key = UniqueKey(); + WithEnvironmentVariable(key, @"C:\Some\Overridden\Path", () => + { + var settings = UninitializedAppSettings(); + + Assert.Equal(@"C:\Some\Overridden\Path", settings.Get(key)); + }); + } + + [Theory] + [InlineData("True", true)] + [InlineData("False", false)] + public void GetBool_WithEnvironmentOverride_ParsesThroughSameConversionPathAsConfig(string envValue, bool expected) + { + var key = UniqueKey(); + WithEnvironmentVariable(key, envValue, () => + { + var settings = UninitializedAppSettings(); + + Assert.Equal(expected, settings.Get(key)); + }); + } + + [Fact] + public void GetInt_WithEnvironmentOverride_ParsesThroughSameConversionPathAsConfig() + { + var key = UniqueKey(); + WithEnvironmentVariable(key, "42", () => + { + var settings = UninitializedAppSettings(); + + Assert.Equal(42, settings.Get(key)); + }); + } + + [Fact] + public void GetInt_WithUnparsableEnvironmentOverride_ReturnsDefaultRatherThanThrowing() + { + // Mirrors the existing (unchanged) catch-all behavior for an unparsable + // config-sourced value - an env-sourced value that fails Parse must fail the + // same safe way, not bypass it. + var key = UniqueKey(); + WithEnvironmentVariable(key, "not-a-number", () => + { + var settings = UninitializedAppSettings(); + + Assert.Equal(0, settings.Get(key)); + }); + } + + // GetUninitializedObject skips AppSettings' constructor (and therefore its + // Environment.Exit(1)-on-missing-config-file check) entirely. Only safe to call + // Get/Get on the result while an environment-variable override is in effect for + // the key under test - GetRawValue returns the override before ever touching the + // lazily-initialized CachedConfig property (and, transitively, AppState.Notifier). + static AppSettings UninitializedAppSettings() + { + return (AppSettings)RuntimeHelpers.GetUninitializedObject(typeof(AppSettings)); + } + + static string UniqueKey() => "TestKey_" + Guid.NewGuid().ToString("N"); + + static void WithEnvironmentVariable(string key, string value, Action action) + { + var envVarName = AppSettings.EnvironmentVariablePrefix + key; + + // Captures and restores whatever was already set, rather than unconditionally + // clearing it in the finally block below - GetEnvironmentOverride_WorksForAnyKey_ + // NoPerKeyCodeNeeded deliberately parameterizes over real production key names + // (GameDirectory, ModsDirectory, ...), so if the test process ever legitimately + // inherited one of those (e.g. a CI job that also drives the CLI merge pipeline in + // the same shell session - exactly this feature's own intended use), unconditional + // clearing would silently wipe that override for the rest of the process instead + // of restoring it. + var originalValue = Environment.GetEnvironmentVariable(envVarName); + Environment.SetEnvironmentVariable(envVarName, value); + try + { + action(); + } + finally + { + Environment.SetEnvironmentVariable(envVarName, originalValue); + } + } + } +}