test(api): cover /health ResponseWriter; map Degraded/Unhealthy to 503 - #379
Conversation
#243) Extract the /health ResponseWriter into a unit-testable internal method that now sets the HTTP status explicitly (200 Healthy / 503 Degraded or Unhealthy) instead of relying on the default status-code map, which returned 200 for a Degraded report and hid stale background services from load balancers and uptime monitors. Enrich the per-check body with description and keep the raw exception object out of the response so failure detail never leaks. Tests build a HealthReport in-memory and invoke the writer directly: healthy => 200 + full body, unhealthy DB => 503 + failing check surfaced, degraded => 503, and a check carrying an exception surfaces its description without leaking the exception message, type, or stack trace. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
There was a problem hiding this comment.
Code Review: PR #379 (thomasluizon/orbit-api)
Scope: PR #379 — test(api): cover /health ResponseWriter; map Degraded/Unhealthy to 503
Recommendation: APPROVE
Summary
Small, well-scoped PR: extracts the inline /health ResponseWriter lambda into a testable internal static WriteHealthCheckResponseAsync(HttpContext, HealthReport) method in src/Orbit.Api/Extensions/WebApplicationExtensions.cs, fixes the previously-wrong status-code mapping (framework default returned 200 for Degraded; now Healthy→200, Degraded/Unhealthy→503), and adds description to each check in the JSON body. Adds 6 focused unit tests in tests/Orbit.Infrastructure.Tests/Health/HealthCheckResponseTests.cs. No prior reviews or review comments exist on this PR — nothing being re-flagged.
Findings
Critical
None.
High
None.
Medium
None.
Low / Info
- [Info] Test class lives in
Orbit.Infrastructure.Testseven though it testsOrbit.Apicode — this is the established, intentional pattern:Orbit.Api.csprojalready declaresInternalsVisibleTo Include="Orbit.Infrastructure.Tests"(pre-existing, not added by this PR), andtests/CLAUDE.mddocumentsOrbit.Infrastructure.Testsas the home for "services, prompt sections, controllers, MCP tools." No action needed. - [Info, from security-reviewer] The newly-serialized
Descriptionfield is safe today — both registered health checks (DatabaseHealthCheck,BackgroundServiceHealthCheck) only ever populateDescriptionwith static strings or internal background-job names, never exception messages or connection strings. Thedatadictionary (per-service tick timestamps) is confirmed not serialized by the writer — only.Descriptionis read. Worth keeping in mind for futureIHealthCheckauthors (don't putex.MessageintoDescription), but not a defect in this diff.
Subagents
| Agent | Verdict |
|---|---|
| security-reviewer | PASS — no data-exposure or auth issues; /health stays AllowAnonymous() per documented exemption; entry.Exception never serialized |
| contract-aligner | N/A — no DTO, Controller route, or packages/shared type changed; /health's JSON shape is an internal ops/monitoring surface, not part of the cross-repo contract |
Validation
| Check | Result |
|---|---|
| Build (dotnet) | N/A — covered by separate required CI checks (Build / Unit Tests / SonarCloud) |
| Tests (dotnet) | N/A — covered by separate required CI checks (Build / Unit Tests / SonarCloud) |
What's good
- Root-causes the actual bug (wrong
Degraded→200 mapping) rather than patching around it; matches howHealthCheckMiddlewarepre-setsStatusCodefromResultStatusCodesbefore invokingResponseWriter— the explicit override in the writer is the correct fix point. - Test suite is genuinely thorough for the surface: covers all three
HealthStatusvalues, the JSON content-type, per-check field surfacing, an empty-report edge case, and a dedicated test proving anException's message/type/stack trace never leak into the public unauthenticated response body. - No narration comments; the one comment present is an XML
///-style doc block, which is allowed by the comment-policy dimension. - Minimal diff, single responsibility, no scope creep.
Recommendation
Merge as-is. No blocking issues found.



What
Closes a
/prod-readinesstests-audit gap: the/healthResponseWriter had no direct test coverage and its status-code behaviour was wrong for degraded reports.Source fix (minimal, required to make the writer testable and correct)
/healthResponseWriterlambda into an internalWriteHealthCheckResponseAsync(HttpContext, HealthReport)method so it can be unit-tested in isolation.200forHealthy,503forDegradedandUnhealthy. Previously it relied on the defaultResultStatusCodesmap, which returns 200 forDegraded— so stale background services (BackgroundServiceHealthCheckreturnsDegraded) read as fully live to a load balancer or uptime monitor.description; the rawentry.Exceptionis still never serialized, so failure detail (messages, stack traces) can't leak.Tests —
tests/Orbit.Infrastructure.Tests/Health/HealthCheckResponseTests.csEach builds a
HealthReportin-memory and invokes the writer directly against aDefaultHttpContext:200, JSON content type,status: "Healthy", and every check's name/status/description surfaced.503, failing check + description surfaced.503(this is the mapping the old code got wrong).SUPERSECRET_TOKEN), type name, andstackTraceare absent from the body.200with no checks.All 5240 API tests pass locally; no narration comments (ORBIT0001).
Refs thomasluizon/orbit-ui-mobile#243