From 3e0f12453b80244ecfc65cecc68358f9c955889d Mon Sep 17 00:00:00 2001 From: Simon Cropp Date: Sat, 22 Aug 2026 12:41:48 +1000 Subject: [PATCH] Let a derived tool inherit useShellExecute Every optional flag on AddToolBasedOn is a nullable that falls back to the tool being derived from - except useShellExecute, which defaulted to true. So its `?? existing.UseShellExecute` was unreachable, and a caller who said nothing about it did not inherit: they got true. Five definitions set it false, and they are the ones where it matters: the bundled viewer, VS Code, Cursor, MsWordDiff and MsExcelDiff. Deriving from any of them silently changed how the tool launches, and took the inherited CreateNoWindow with it - which on the viewer is the console flash the definition exists to prevent. The test asserts the defaults rather than deriving a tool, and that is deliberate. AddToolBasedOn resolves through ToolLookup, so it only works for a tool actually installed on the machine, and the viewer resolves on neither this machine nor CI - I wrote the behavioural version first and it passed against the bug, because it was taking its own early return. Asserting that every bool? flag defaults to null pins the real contract and fails without the fix. --- src/DiffEngine.Tests/AddToolBasedOnTests.cs | 48 +++++++++++++++++++++ src/DiffEngine/DiffTools_Add.cs | 6 ++- 2 files changed, 53 insertions(+), 1 deletion(-) create mode 100644 src/DiffEngine.Tests/AddToolBasedOnTests.cs diff --git a/src/DiffEngine.Tests/AddToolBasedOnTests.cs b/src/DiffEngine.Tests/AddToolBasedOnTests.cs new file mode 100644 index 00000000..eedc01ca --- /dev/null +++ b/src/DiffEngine.Tests/AddToolBasedOnTests.cs @@ -0,0 +1,48 @@ +/// +/// What a derived tool inherits. +/// +/// Every flag on AddToolBasedOn is a nullable that falls back to the tool being derived from, and +/// useShellExecute defaulted to true rather than null - so its `?? existing` was unreachable, and a +/// caller who said nothing about it got true rather than what they derived from. For the five +/// definitions that set it false (the bundled viewer, VS Code, Cursor, MsWordDiff, MsExcelDiff) +/// that silently changed how the tool launches, and took the inherited CreateNoWindow with it, +/// which is the console flash the viewer definition exists to prevent. +/// +/// +/// Asserted on the defaults rather than on a derived tool, because AddToolBasedOn resolves through +/// ToolLookup and so only works for a tool installed on the machine running the test. A behavioural +/// test would quietly do nothing wherever the tool it names is absent - which, for the viewer, is +/// this machine and CI both. +/// +/// +public class AddToolBasedOnTests +{ + [Test] + public async Task EveryOptionalFlagFallsBackToTheToolItIsBasedOn() + { + var method = typeof(DiffTools).GetMethod(nameof(DiffTools.AddToolBasedOn))!; + + var withDefaults = method + .GetParameters() + .Where(_ => _.ParameterType == typeof(bool?)) + .ToList(); + + // The flags, so a new one cannot be added without this noticing + await Assert.That(withDefaults.Select(_ => _.Name!).ToList()).IsEquivalentTo( + [ + "autoRefresh", + "isMdi", + "supportsText", + "requiresTarget", + "useShellExecute", + "createNoWindow", + "killLockingProcess" + ]); + + foreach (var parameter in withDefaults) + { + await Assert.That(parameter.HasDefaultValue).IsTrue(); + await Assert.That(parameter.DefaultValue).IsNull(); + } + } +} diff --git a/src/DiffEngine/DiffTools_Add.cs b/src/DiffEngine/DiffTools_Add.cs index abf07fda..9b9af4db 100644 --- a/src/DiffEngine/DiffTools_Add.cs +++ b/src/DiffEngine/DiffTools_Add.cs @@ -8,7 +8,11 @@ public static partial class DiffTools bool? isMdi = null, bool? supportsText = null, bool? requiresTarget = null, - bool? useShellExecute = true, + // Null like every other flag here. Defaulting to true made the `?? existing` below dead + // code for this one alone, so a tool based on one of the five definitions that set it + // false - the bundled viewer, VS Code, Cursor, MsWordDiff, MsExcelDiff - silently became + // a ShellExecute launch, and took the inherited CreateNoWindow with it + bool? useShellExecute = null, bool? createNoWindow = null, bool? killLockingProcess = null, LaunchArguments? launchArguments = null,