From 2981e839ca73dbe2ab403714ca531615b67a0d5f Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 21 Sep 2026 20:29:07 +0000 Subject: [PATCH] fix: reject a null success value in GitResult.FromValue [minor] MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GitResult.Success is derived from Error alone, so FromValue(null) produced an instance reporting Success with a null Value — precisely the state the type's own remarks promise cannot occur. FromError already guarded against the mirror case with Ensure.NotNull; FromValue guarded nothing. The path there is public: GitCommandBuilder is unsealed with no constraint on TResult, so a derived builder whose ParseResult returns null on some edge case reached FromValue with null and turned a would-be NullReferenceException into a silently successful result. Constrain the type parameter to notnull on GitResult, IGitCommandBuilder and GitCommandBuilder, and guard FromValue at run time as well, since the constraint is a compile-time check a caller can suppress. The constraint stops short of class because two result types are value types: rev-list counts commits into an int, and Divergence() answers with a GitDivergence struct. For the same reason the run-time guard reaches Ensure.NotNull through an object cast, which the value-type instantiations fold away rather than boxing on every result. Fixes #116 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01NPXBwr1rm8hybVmmWhx7wd --- .../Execution/GitResultTests.cs | 16 +++++++++ GitIntegration/Builders/GitCommandBuilder.cs | 7 +++- GitIntegration/Builders/IGitCommandBuilder.cs | 6 +++- GitIntegration/Execution/GitResult.cs | 36 ++++++++++++++++++- 4 files changed, 62 insertions(+), 3 deletions(-) diff --git a/GitIntegration.Test/Execution/GitResultTests.cs b/GitIntegration.Test/Execution/GitResultTests.cs index eda7d10..84e72d6 100644 --- a/GitIntegration.Test/Execution/GitResultTests.cs +++ b/GitIntegration.Test/Execution/GitResultTests.cs @@ -65,6 +65,22 @@ public void FromErrorRejectsNullError() _ = Assert.ThrowsExactly(() => GitResult.FromError(null!)); } + [TestMethod] + public void FromValueRejectsNullValue() + { + _ = Assert.ThrowsExactly(() => GitResult.FromValue(null!)); + } + + [TestMethod] + public void FromValueRejectsNullValueForAnyReferenceType() + { + // GitCommandBuilder is public, unsealed, and puts no notnull constraint on TResult, + // so a derived builder whose ParseResult returns null on an edge case reaches FromValue with + // null for whatever reference type it closed over — not just string. + _ = Assert.ThrowsExactly( + () => GitResult>.FromValue(null!)); + } + [TestMethod] public void ProcessResultReportsSuccessOnZeroExitCode() { diff --git a/GitIntegration/Builders/GitCommandBuilder.cs b/GitIntegration/Builders/GitCommandBuilder.cs index e75be0b..9b14aae 100644 --- a/GitIntegration/Builders/GitCommandBuilder.cs +++ b/GitIntegration/Builders/GitCommandBuilder.cs @@ -13,7 +13,11 @@ namespace ktsu.GitIntegration; /// The shared behaviour of every git command builder: global argument injection, execution, and /// failure translation. /// -/// The parsed result type. +/// +/// The parsed result type. Constrained to notnull so that a derived builder whose +/// returns null on some edge case cannot yield a successful-looking +/// with a null value; throws instead. +/// /// Runs the assembled command. /// /// The repository to scope the command to, or for commands that are not @@ -21,6 +25,7 @@ namespace ktsu.GitIntegration; /// public abstract class GitCommandBuilder(IGitProcessRunner runner, AbsoluteDirectoryPath? repositoryPath) : IGitCommandBuilder + where TResult : notnull { /// /// Gets the runner this builder executes through. diff --git a/GitIntegration/Builders/IGitCommandBuilder.cs b/GitIntegration/Builders/IGitCommandBuilder.cs index 27c9a79..af6c228 100644 --- a/GitIntegration/Builders/IGitCommandBuilder.cs +++ b/GitIntegration/Builders/IGitCommandBuilder.cs @@ -16,8 +16,12 @@ namespace ktsu.GitIntegration; /// a fresh builder per command. The underlying is the opposite: it /// is a shared singleton and safe to call concurrently. /// -/// The parsed result type. +/// +/// The parsed result type. Constrained to notnull because reports +/// success purely by the absence of an error, so it cannot represent a null success value. +/// public interface IGitCommandBuilder + where TResult : notnull { /// /// Gets the exact argument vector this builder will pass to git. diff --git a/GitIntegration/Execution/GitResult.cs b/GitIntegration/Execution/GitResult.cs index 648b938..04dcaed 100644 --- a/GitIntegration/Execution/GitResult.cs +++ b/GitIntegration/Execution/GitResult.cs @@ -24,6 +24,7 @@ public sealed record GitCommandError /// /// The parsed result type on success. /// +/// /// This is a sealed record class, not a struct, so that there is no default instance to /// accidentally observe. A struct with independently-set members would let /// default(GitResult<T>) — reachable via an uninitialized field, an array, or a failed @@ -32,8 +33,19 @@ public sealed record GitCommandError /// reasonably dereferences the field matching the branch it is on. Restricting construction to /// and makes every instance one /// or the other, never both undefined. +/// +/// +/// is constrained to notnull, and +/// rejects a null value the way rejects a null error. +/// Without both, FromValue(null) would report with nothing for the +/// success branch to read — the one case the guarantee above does not survive. The constraint stops +/// short of class because two result types are value types: rev-list counts commits +/// into an , and Divergence() answers with a +/// struct. +/// /// public sealed record GitResult + where T : notnull { private GitResult() { } @@ -49,7 +61,29 @@ private GitResult() { } /// Creates a successful result. /// The parsed result. /// A successful result carrying . - public static GitResult FromValue(T value) => new() { Value = value }; + /// + /// is . is derived from + /// alone, so a null value here would report success while leaving nothing for + /// the success branch to read — the one reading this type's remarks promise is safe. + /// + public static GitResult FromValue(T value) + { + // The notnull constraint is a compile-time check a caller can suppress, and the extension + // point this guards — a derived GitCommandBuilder whose ParseResult returns null — is + // reachable from outside this assembly, so the invariant is enforced at run time too. + // + // Ensure.NotNull constrains its own argument to a reference type, which T is not: the + // constraint above stops at notnull for GitResult and GitResult. Casting + // to object satisfies that constraint for the reference-type instantiations, where null is + // the only place it is reachable. typeof(T).IsValueType is a per-instantiation constant, so + // the value-type instantiations fold this check away instead of boxing on every result. + if (!typeof(T).IsValueType) + { + Ensure.NotNull((object?)value, nameof(value)); + } + + return new() { Value = value }; + } /// Creates a failed result. /// The failure detail.