fix: guard against IndexOutOfRangeException when Branches list is empty in GithubStatusNotificationHandler

[dnyw4l3n13]

Summary

• Replaced the unchecked message.Branches[^1] access in BuildStatusMessage with a null-guarded form (message.Branches.Count > 0 ? message.Branches[^1] : null), preventing IndexOutOfRangeException when the GitHub status payload arrives with an empty Branches list.
• Updated the embed title interpolation to use lastBranch?.Name ?? string.Empty so an empty branches list produces a valid (if branch-less) title.
• Fixed AddBranchEmbed to use ?? "(unknown)" on the FirstOrDefault() result so WithValue never receives null or empty (Discord rejects both).
• Added parameterised test cases (NonPendingStateWithNoBranchesShouldPublishAsync) covering success, failure, and error states with an empty branches list.

Test plan

• All existing tests continue to pass (non-empty branches path unchanged).
• New NonPendingStateWithNoBranchesShouldPublishAsync tests pass for success, failure, and error states.
• CI build passes.

Closes #365

[dnyw4l3n13]

Code Review: fix: guard against IndexOutOfRangeException when Branches list is empty in GithubStatusNotificationHandler

Summary

Targeted bug fix in GithubStatusNotificationHandler with appropriate test coverage. Low-medium blast radius — changes are confined to a single internal handler. The fix is correct; one minor inconsistency found.

Suggestions (nice to have)

• Inconsistent empty-branch fallback text — GithubStatusNotificationHandler.cs:60
  The title uses string.Empty when branches is empty (lastBranch?.Name ?? string.Empty), producing a Discord embed title like "Build finished for ci/build from BuildBot ()". The embed field (line 77) uses "(unknown)", which is more readable. Both fallbacks should use "(unknown)" for consistency and user clarity.

Architecture Compliance

PASS — all changes are internal to BuildBot.GitHub and its test project; no layer violations.

Test Coverage

New test NonPendingStateWithNoBranchesShouldPublishAsync correctly exercises the empty-branches path and verifies no exception is thrown and publishing still proceeds. Adequate for the scope of this fix.

What's Good

• Minimal, focused fix that addresses exactly the reported crash scenario
• No over-engineering — correct use of nullable reference type (Branch?) and null-coalescing
• AddBranchEmbed correctly uses FirstOrDefault() ?? "(unknown)" avoiding both exceptions and null embed values
• Test helper refactoring (MakeStatusWithBranches) is clean and reusable

[dnyw4l3n13]

Code review round: fixed 1 finding — used consistent '(unknown)' fallback in status message title (c280a9f)

[dnyw4l3n13]

Code Review: fix: guard against IndexOutOfRangeException when Branches list is empty in GithubStatusNotificationHandler

Summary

Clean, focused bug fix with full test coverage. Low blast radius — changes confined to GithubStatusNotificationHandler and its test project. All 36 tests pass locally. No new findings.

Critical (must fix before merge)

None.

Warnings (should fix, creates tech debt)

None.

Suggestions (nice to have)

None — the previous round's finding (inconsistent fallback text) was already fixed in c280a9f.

Architecture Compliance

PASS — all changes internal to BuildBot.GitHub and BuildBot.GitHub.Tests; no layer violations.

Test Coverage

PASS — NonPendingStateWithNoBranchesShouldPublishAsync correctly exercises the empty-branches path for all non-pending states (success, failure, error). Consistent pattern with existing tests.

What's Good

• Minimal, correct fix using nullable reference type + null-coalescing
• Both fallbacks (title and Branch field) now consistently show "(unknown)" for the empty-branches case
• Test helper refactoring (MakeStatusWithBranches) is clean and reusable
• No over-engineering

[dnyw4l3n13]

Code review clean — advancing to security review

[dnyw4l3n13]

Code Review: fix: guard against IndexOutOfRangeException when Branches list is empty in GithubStatusNotificationHandler

Summary

Clean, minimal bug fix with correct test coverage. Low blast radius — changes confined to GithubStatusNotificationHandler and its test project. CI passes. No new findings.

Critical (must fix before merge)

None.

Warnings (should fix, creates tech debt)

None.

Suggestions (nice to have)

None.

Architecture Compliance

PASS — all changes internal to BuildBot.GitHub and BuildBot.GitHub.Tests; no layer violations.

Test Coverage

PASS — NonPendingStateWithNoBranchesShouldPublishAsync correctly exercises the empty-branches path for all non-pending states (success, failure, error). Consistent pattern with existing tests. Pending state short-circuits before BuildStatusMessage is called, so no test is needed for that path.

What's Good

• Minimal, correct fix: guards Branches[^1] access via .Count > 0 check
• Both fallbacks (title and Branch embed field) consistently use "(unknown)" for the empty-branches case
• AddBranchEmbed correctly uses FirstOrDefault() ?? "(unknown)" — avoids both exception and null embed value
• Test helper refactoring (MakeStatusWithBranches) is clean and reusable across existing and new test cases
• No over-engineering

[dnyw4l3n13]

Code review clean — advancing to security review

[dnyw4l3n13]

Security review clean — advancing to human review

[dnyw4l3n13]

PR marked ready for review and auto-merge enabled — will merge once all required checks pass and review is approved.

[credfeto]

Super-linter summary

Language	Validation result

All files and directories linted successfully

For more information, see the GitHub Actions workflow run

Powered by Super-linter