fix: reject a null success value in GitResult.FromValue [minor]

[matt-edmondson]

Fixes #116

The problem

GitResult<T>.Success is derived from Error alone, so FromValue(null) produced an instance reporting Success == true with Value == null — precisely the state the type's own <remarks> promise cannot occur ("Neither reading is safe for a consumer that reasonably dereferences the field matching the branch it is on"). FromError already guarded the mirror case with Ensure.NotNull; FromValue guarded nothing.

The path there is public, not hypothetical: GitCommandBuilder<TResult> is unsealed with no constraint on TResult, and its TryExecuteAsync calls GitResult<TResult>.FromValue(ParseResult(result)). A derived builder whose ParseResult returns null on some edge case turned a would-be NullReferenceException into a silently successful result.

The fix

Both halves of what the issue proposed, because neither alone is sufficient:

• Compile time — where T : notnull on GitResult<T>, IGitCommandBuilder<TResult> and GitCommandBuilder<TResult>, so the extension point is constrained at the signature.
• Run time — FromValue now rejects null, since the constraint is a compile-time check a caller can suppress (FromValue(null!)) and the derived-builder seam is reachable from outside this assembly.

Two details worth flagging for review:

• The constraint stops at notnull, not class. I tried class first and the compiler rejected it: IGitRevListBuilder : IGitCommandBuilder<int> and IGitRevListDivergenceBuilder : IGitCommandBuilder<GitDivergence> (a readonly record struct) are both real instantiations, so GitResult<int> and GitResult<GitDivergence> exist today. notnull admits both.
• The guard reaches Ensure.NotNull through an object cast. Ensure.NotNull constrains its own argument to a reference type, which T is not, and the KTSU0003/KTSU0004 analyzers reject both ArgumentNullException.ThrowIfNull and a manual if (value is null) throw. Casting to object satisfies the constraint for the reference-type instantiations, where null is the only place it is reachable, behind a typeof(T).IsValueType test that is a per-instantiation constant — so the value-type instantiations fold the check away instead of boxing on every result. No suppression was needed, keeping this repo's zero-[SuppressMessage] record intact.

Testing

Two tests added to GitResultTests, mirroring the existing FromErrorRejectsNullError: one for GitResult<string>, one for GitResult<IReadOnlyList<string>> to cover the derived-builder scenario over a non-string reference type.

Verified they actually catch the bug by reverting FromValue to its original expression body and re-running: both fail with "Expected exception of exact type ArgumentNullException but no exception was thrown", and the other 11 tests in the class still pass. With the fix restored, the full suite is green — 610 passed, 0 failed.

Tagged [minor] rather than [patch]: the public signatures changed. It is not a compile break for consumers, since violating notnull is a warning (CS8714) rather than an error.

🤖 Generated with Claude Code

https://claude.ai/code/session_01NPXBwr1rm8hybVmmWhx7wd

---

Generated by Claude Code

[sonarqubecloud]

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
100.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[matt-edmondson]

CI green, and a note on the one SonarCloud finding

All 14 checks pass, including the test legs on ubuntu, windows and macOS, and the quality gate passed with 100% coverage on new code.

The gate did report 1 new issue — a MINOR code smell on GitResult.cs:82:

Remove this argument from the method call; it hides the caller information.

That is rule S3236 firing on the nameof(value) in Ensure.NotNull((object?)value, nameof(value)). I've left it deliberately, because here the rule's premise is inverted: that parameter is a [CallerArgumentExpression] one, and the argument being passed is a cast, so the compiler-supplied text would be the literal source expression (object?)value. Omitting nameof(value) would therefore produce

ArgumentNullException: Value cannot be null. (Parameter '(object?)value')

rather than (Parameter 'value'). Passing it explicitly improves the caller information instead of hiding it, and keeps the paramName matching the actual parameter — and FromError's behaviour on the same class.

Happy to drop it if you'd rather have a clean Sonar report than the accurate parameter name, but that is the trade being made. The cast itself is unavoidable while T is constrained only to notnull, which it must be: GitResult<int> and GitResult<GitDivergence> are both real instantiations.

---

Generated by Claude Code