fix: correct inverted null guard on client settings attachments

[bobbyg603]

Closes #161

Problem

Runtime/Util/ReportPostOptionsExtensions.cs guarded the attachment copy with:

if (clientSettings.Attachments?.Count != 0)
{
    options.AdditionalAttachments.AddRange(clientSettings.Attachments);
}

When Attachments is null, ?.Count yields a null int? and null != 0 is true, so the guard falls through into AddRange(null) and throws ArgumentNullException. The guard was exactly inverted: it skipped the copy only in the one case where copying is harmless (an empty list) and admitted it in the case that throws.

Fix

if (clientSettings.Attachments?.Count > 0)

> on a lifted int? is false when the operand is null, so a null Attachments correctly skips the copy.

Verification of the "latent today" claim

Confirmed still latent on main — both shipped settings repositories initialize the list:

• WebGLClientSettingsRepository.Attachments is initialized inline (= new List<FileInfo>()).
• DotNetStandardClientSettingsRepository.Attachments delegates to BugSplatDotNetStandard.BugSplat.Attachments. I instantiated the vendored Runtime/Plugins/BugSplatDotNetStandard.dll directly and confirmed the property is non-null (empty list) on a fresh instance.

So nothing crashes today, but any new IClientSettingsRepository implementation that leaves the property null would throw on every report post.

Other collections in the file

I checked the rest of SetNullOrEmptyValues and grepped Runtime/ and Editor/ for ?.Count / ?.Length. The inverted guard appeared exactly once — the line fixed here is the only instance in the repo.

One adjacent observation, left unchanged to keep this fix minimal: foreach (var attribute in clientSettings.Attributes) has no null guard at all, so it would NullReferenceException under the same "future repository leaves it null" scenario. Same latency argument (both repos initialize Attributes), different pattern from the inverted guard this issue describes. Happy to fold it in if a reviewer prefers.

Tests

Tests/Runtime/Util/ExceptionPostOptionsExtensionsTests.cs was an empty TODO stub — precisely the file that should have caught this. Filled it with coverage scoped to the null-guard behaviour:

• SetNullOrEmptyValues_NullAttachments_ShouldNotThrow
• SetNullOrEmptyValues_EmptyAttachments_ShouldNotAddAttachments
• SetNullOrEmptyValues_PopulatedAttachments_ShouldAddAttachments
• Attachments_ShippedClientSettingsRepository_ShouldBeInitialized (regression guard on the "latent" premise)

The null test is a genuine regression test: reverted to != 0, it fails with exactly System.ArgumentNullException: Value cannot be null. (Parameter 'collection') at List.AddRange / ReportPostOptionsExtensions.cs:12. Restored, it passes.

Note

#167 is concurrently expanding this same file (it fills both empty stubs, ShouldPostExceptionImplTests.cs and ExceptionPostOptionsExtensionsTests.cs). I kept this change tightly scoped to the null-guard behaviour, but expect a small merge in ExceptionPostOptionsExtensionsTests.cs.

How I verified

Unity's test runner was not available here, so I compiled and ran these types out-of-editor — they are plain C# with no Unity runtime dependency.

1. Full compile: adapted pkgcheck.csproj (whole Runtime/** + Tests/Runtime/** against the Unity managed DLLs and the vendored BugSplatDotNetStandard.dll) — dotnet build succeeds, 0 errors, only the two pre-existing CS0649 warnings in BugSplat.cs.
2. Actually ran the tests: a second project compiling the non-Unity-test-infra subset (ReportPostOptions, the settings repositories, ReportPostOptionsExtensions, ShouldPostExceptionImpl, NativeSyncDictionary, plus ExceptionPostOptionsExtensionsTests and NativeSyncDictionaryTests) with NUnit 3.14 + NUnit3TestAdapter + Microsoft.NET.Test.Sdk.

Passed!  - Failed: 0, Passed: 15, Skipped: 0, Total: 15

15 = my 4 new tests + the 11 existing NativeSyncDictionaryTests. The [UnityTest]-based suites need the Unity editor and were compile-checked only, not executed.

🤖 Generated with Claude Code

[Copilot]

Pull request overview

Fixes a latent ArgumentNullException in ReportPostOptionsExtensions.SetNullOrEmptyValues by correcting an inverted null/empty guard when copying client settings attachments, and adds runtime-unit tests to prevent regression.

Changes:

• Corrected attachments guard from != 0 to > 0 to properly skip null/empty attachment lists.
• Added NUnit tests covering null, empty, and populated attachments behavior, plus a regression check for shipped settings initialization.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File	Description
Runtime/Util/ReportPostOptionsExtensions.cs	Fixes the inverted null/empty guard to prevent AddRange(null) when attachments are unset.
Tests/Runtime/Util/ExceptionPostOptionsExtensionsTests.cs	Adds regression tests for SetNullOrEmptyValues attachment handling.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.