From cd6eefd0bacf123df9db3230222746ef2647e9fe Mon Sep 17 00:00:00 2001 From: Erik Date: Thu, 20 Aug 2026 21:28:04 +0200 Subject: [PATCH] feat(plugins): enforce apiVersion; launcher plugins default ON with "none" opt-out MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two gaps from the MossTank shipment review. **apiVersion was declared in every manifest and checked by nothing.** The loader now refuses an unsupported contract BEFORE loading any code from the plugin — checking after the fact is not equivalent, because by then the assembly is in a collectible context and the mismatch surfaces as a type-load or missing-member failure from inside the plugin, which reads like the plugin is broken rather than built for a different host. PluginApi (Current / MinimumSupported) lives in Plugin.Abstractions beside the contract it versions, and the refusal is a distinct PluginApiVersionException so callers can tell "update the client or the plugin" from "this plugin is broken". The tests pin the ordering too: a manifest with a future apiVersion AND a missing dll must fail on the version, a supported one on the dll. **A launcher-launched client loaded no plugins until the user typed ids.** LA5 distinguishes an omitted allow-list (load all) from an explicit empty one (load none); a fresh character profile's list is empty, so it composed to load-none. Direct launches pass null and load everything -- which is why the gap never showed in development: the two launch paths disagreed and the launcher was the one users get. This REVERSES the LA5 default deliberately: "nothing configured" now composes to the omitted list, so plugins are on by default, including ones installed later. The opt-out is kept -- losing it would be a real regression for stripped sessions -- respelled as the literal id "none", and the launcher's plugin box says so. The cross-host shared fixture composes its explicit-load-none case through the new spelling, keeping the reader-side contract tests (App and Headless both preserve an explicit empty list) exactly as they were. Complete Release suite: 14,469 tests pass on the standard hermetic lane filter, 0 failures. Co-Authored-By: Claude Opus 5 --- src/AcDream.Core/Plugins/PluginLoader.cs | 15 ++++++ src/AcDream.Core/Plugins/PluginManifest.cs | 10 ++++ .../Launching/SessionConfigComposer.cs | 39 ++++++++++++-- src/AcDream.Launcher/MainWindow.axaml | 2 +- .../IAcDreamPlugin.cs | 33 ++++++++++++ .../Plugins/PluginLoaderTests.cs | 43 +++++++++++++++ .../Launching/SessionConfigComposerTests.cs | 54 ++++++++++++++++++- .../LauncherCoreSessionConfigFixture.cs | 5 +- 8 files changed, 194 insertions(+), 7 deletions(-) diff --git a/src/AcDream.Core/Plugins/PluginLoader.cs b/src/AcDream.Core/Plugins/PluginLoader.cs index 1d729f2a..7683f705 100644 --- a/src/AcDream.Core/Plugins/PluginLoader.cs +++ b/src/AcDream.Core/Plugins/PluginLoader.cs @@ -20,6 +20,21 @@ public static class PluginLoader ArgumentNullException.ThrowIfNull(manifest); ArgumentNullException.ThrowIfNull(host); + // Refuse a contract we cannot honour BEFORE loading any code from it. + // Checking after the fact is not equivalent: the assembly is already in + // a collectible context, and the mismatch surfaces as a type-load or + // missing-member failure from inside the plugin, which reads like the + // plugin is broken rather than built for a different host. + if (!PluginApi.IsSupported(manifest.ApiVersion)) + return new LoadedPlugin( + manifest, + Plugin: null, + LoadContext: null, + Error: new PluginApiVersionException( + $"plugin '{manifest.Id}' declares apiVersion {manifest.ApiVersion}, " + + $"but this build supports {PluginApi.MinimumSupported}" + + $"..{PluginApi.Current}")); + var dllPath = Path.Combine(pluginDirectory, manifest.EntryDll); if (!File.Exists(dllPath)) return new LoadedPlugin( diff --git a/src/AcDream.Core/Plugins/PluginManifest.cs b/src/AcDream.Core/Plugins/PluginManifest.cs index da16dcfa..a6cfbd97 100644 --- a/src/AcDream.Core/Plugins/PluginManifest.cs +++ b/src/AcDream.Core/Plugins/PluginManifest.cs @@ -64,6 +64,16 @@ public sealed record PluginManifest( } } +/// +/// A plugin declared a contract version this build cannot load. Distinct from +/// (a malformed manifest) because the +/// remedy differs: this one means update the plugin or the client. +/// +public sealed class PluginApiVersionException : Exception +{ + public PluginApiVersionException(string message) : base(message) { } +} + public sealed class PluginManifestException : Exception { public PluginManifestException(string message) : base(message) { } diff --git a/src/AcDream.Launcher.Core/Launching/SessionConfigComposer.cs b/src/AcDream.Launcher.Core/Launching/SessionConfigComposer.cs index 7d9eb594..d691ff1d 100644 --- a/src/AcDream.Launcher.Core/Launching/SessionConfigComposer.cs +++ b/src/AcDream.Launcher.Core/Launching/SessionConfigComposer.cs @@ -138,9 +138,7 @@ public static class SessionConfigComposer Character = selector, Policy = policy, Credential = new SessionCredentialDescriptor(), - // LA5 distinguishes an omitted allow-list (load all, preserving - // the developer flow) from an explicit empty list (load none). - Plugins = [.. character.Plugins], + Plugins = ComposePluginAllowList(character.Plugins), LoginCommands = character.LoginCommands.Count > 0 ? [.. character.LoginCommands] : null, @@ -271,6 +269,41 @@ public static class SessionConfigComposer string sessionId) => Write(ComposeProbe(server, account, install, paths, sessionId)); + /// + /// Maps a character's configured plugin ids to the session config's + /// allow-list. + /// + /// + /// + /// LA5 distinguishes an OMITTED allow-list (load every discovered plugin) + /// from an explicit EMPTY one (load none). A brand-new character profile + /// starts with an empty list, which meant a client that ships plugins + /// loaded none of them until the user typed an id — "ships with the + /// client" and "works out of the box" were different things, and the + /// difference was invisible: nothing was logged, the panel simply never + /// appeared. + /// + /// + /// So "nothing configured" now maps to omitted — plugins are on by + /// default, including ones installed later. The opt-out is kept, because + /// losing it would be a real regression for anyone running a stripped + /// session: the literal id none maps to the explicit empty list. + /// + /// + private static List? ComposePluginAllowList(IReadOnlyList configured) + { + if (configured.Count == 0) + return null; // default: load all + + if (configured.Count == 1 + && string.Equals(configured[0], "none", StringComparison.OrdinalIgnoreCase)) + { + return []; // explicit: load none + } + + return [.. configured]; + } + private static ComposedSessionConfig Write(ComposedSessionConfig composed) { string? directory = Path.GetDirectoryName(composed.ConfigFilePath); diff --git a/src/AcDream.Launcher/MainWindow.axaml b/src/AcDream.Launcher/MainWindow.axaml index 28c73b0e..7d877b3d 100644 --- a/src/AcDream.Launcher/MainWindow.axaml +++ b/src/AcDream.Launcher/MainWindow.axaml @@ -203,7 +203,7 @@ value is left alone; it is simply no longer offered. --> - + The plugin contract's version. +/// +/// +/// A plugin declares the version it was built against in its +/// plugin.json (apiVersion), and the host refuses to load one +/// it cannot honour. Without that check a plugin built against a different +/// contract loads anyway and fails later as a MissingMethodException or a +/// type-load error from inside the plugin's own code — an error that reads +/// like the plugin is broken rather than mismatched. +/// +/// +/// Bump only for a BREAKING change to the interfaces in +/// this assembly (a removed or re-shaped member). Purely additive changes keep +/// the number, since a plugin built against the older shape still runs. +/// +/// +public static class PluginApi +{ + /// The contract version this build implements. + public const int Current = 1; + + /// + /// The oldest contract version this build can still load. Equal to + /// until a breaking change ships with a + /// compatibility path. + /// + public const int MinimumSupported = 1; + + /// Whether a plugin declaring can load here. + public static bool IsSupported(int apiVersion) + => apiVersion >= MinimumSupported && apiVersion <= Current; +} + public interface IAcDreamPlugin { /// diff --git a/tests/AcDream.Core.Tests/Plugins/PluginLoaderTests.cs b/tests/AcDream.Core.Tests/Plugins/PluginLoaderTests.cs index aa391121..c9b44bbe 100644 --- a/tests/AcDream.Core.Tests/Plugins/PluginLoaderTests.cs +++ b/tests/AcDream.Core.Tests/Plugins/PluginLoaderTests.cs @@ -96,6 +96,49 @@ public class PluginLoaderTests loaded.LoadContext!.Unload(); } + [Fact] + public void Load_UnsupportedApiVersion_IsRefusedBeforeAnyCodeLoads() + { + var host = new StubHost(); + var manifest = new PluginManifest( + Id: "future.plugin", + DisplayName: "Future", + Version: "1.0.0", + EntryDll: "nope.dll", // deliberately nonexistent: + ApiVersion: PluginApi.Current + 1, + Dependencies: Array.Empty()); + + var loaded = PluginLoader.Load("/does/not/exist", manifest, host); + + Assert.False(loaded.Success); + // ...the version gate must fire FIRST, before the dll is even probed, + // so the failure names the real remedy (update the client or the + // plugin) instead of a file-not-found or a type-load error from + // half-loaded plugin code. + var mismatch = Assert.IsType(loaded.Error); + Assert.Contains("future.plugin", mismatch.Message); + Assert.Null(loaded.LoadContext); + } + + [Fact] + public void Load_MinimumSupportedApiVersion_PassesTheGate() + { + var host = new StubHost(); + var manifest = new PluginManifest( + Id: "old.plugin", + DisplayName: "Old", + Version: "1.0.0", + EntryDll: "nope.dll", + ApiVersion: PluginApi.MinimumSupported, + Dependencies: Array.Empty()); + + var loaded = PluginLoader.Load("/does/not/exist", manifest, host); + + // Fails on the missing dll, NOT on the version gate. + Assert.False(loaded.Success); + Assert.IsType(loaded.Error); + } + [Fact] public void Load_MissingDll_ReturnsFailure() { diff --git a/tests/AcDream.Launcher.Core.Tests/Launching/SessionConfigComposerTests.cs b/tests/AcDream.Launcher.Core.Tests/Launching/SessionConfigComposerTests.cs index dae51b3f..8f617c6d 100644 --- a/tests/AcDream.Launcher.Core.Tests/Launching/SessionConfigComposerTests.cs +++ b/tests/AcDream.Launcher.Core.Tests/Launching/SessionConfigComposerTests.cs @@ -174,8 +174,20 @@ public sealed class SessionConfigComposerTests Assert.Equal("+Acdream", (string?)session["character"]!["name"]); } + /// + /// A character nobody has configured loads every discovered plugin. + /// + /// + /// This REVERSES the original LA5 mapping, deliberately. LA5's + /// omitted-vs-empty distinction is kept intact at the session-config + /// level; what changed is which one an unconfigured character maps to. A + /// new profile starts with an empty list, so a client that ships plugins + /// used to load none of them until the user typed an id — and nothing + /// reported it, the panel simply never appeared. The opt-out lives on as + /// the literal id "none" (see the next test). + /// [Fact] - public void EmptyPluginsRemainAnExplicitLoadNoneAllowListWhileLoginCommandsAreOmitted() + public void UnconfiguredPluginsOmitTheAllowListSoEveryPluginLoads() { CharacterProfile character = Character(LaunchMode.Gui); character.Plugins = []; @@ -189,10 +201,48 @@ public sealed class SessionConfigComposerTests Paths, sessionId: "session-empty-lists"); + JsonObject session = SingleSession(composed); + Assert.False(session.ContainsKey("plugins")); + Assert.False(session.ContainsKey("loginCommands")); + } + + [Fact] + public void ThePluginIdNoneEmitsAnExplicitLoadNoneAllowList() + { + CharacterProfile character = Character(LaunchMode.Gui); + character.Plugins = ["none"]; + + ComposedSessionConfig composed = SessionConfigComposer.Compose( + Server(), + Account(), + character, + Install, + Paths, + sessionId: "session-no-plugins"); + JsonObject session = SingleSession(composed); Assert.True(session.ContainsKey("plugins")); Assert.Empty(session["plugins"]!.AsArray()); - Assert.False(session.ContainsKey("loginCommands")); + } + + [Fact] + public void ConfiguredPluginIdsArePassedThroughUnchanged() + { + CharacterProfile character = Character(LaunchMode.Gui); + character.Plugins = ["acdream.mosstank"]; + + ComposedSessionConfig composed = SessionConfigComposer.Compose( + Server(), + Account(), + character, + Install, + Paths, + sessionId: "session-one-plugin"); + + JsonObject session = SingleSession(composed); + Assert.Equal( + ["acdream.mosstank"], + session["plugins"]!.AsArray().Select(node => (string?)node)); } [Fact] diff --git a/tests/Fixtures/campaign-la/LauncherCoreSessionConfigFixture.cs b/tests/Fixtures/campaign-la/LauncherCoreSessionConfigFixture.cs index cc841868..cca943a0 100644 --- a/tests/Fixtures/campaign-la/LauncherCoreSessionConfigFixture.cs +++ b/tests/Fixtures/campaign-la/LauncherCoreSessionConfigFixture.cs @@ -65,7 +65,10 @@ internal static class LauncherCoreSessionConfigFixture Name = "Composer Character", Id = "0x50000001", LaunchMode = LaunchMode.Headless, - Plugins = [], + // "none" is how a profile now spells the explicit load-none + // allow-list; an EMPTY list means unconfigured and composes to an + // OMITTED allow-list (= load all) since plugins became default-on. + Plugins = ["none"], LoginCommands = [], };