feat(gateway): athenahealth integration with encounter polling and PA workflow#18
Merged
feat(gateway): athenahealth integration with encounter polling and PA workflow#18
Conversation
…lient credentials and token caching Implements athenahealth integration foundation: - AthenaOptions configuration record with validation - ITokenAcquisitionStrategy interface for EHR-agnostic token acquisition - AthenaTokenStrategy with OAuth 2.0 client credentials flow and in-memory token caching Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
… and submit endpoint
Implements Group D tasks (D01-D04) for real-time notifications and document submission:
- D01: NotificationHub with unbounded channel for SSE streaming
- D02: /api/events endpoint for Server-Sent Events streaming
- D03: DocumentUploader tests for PDF upload to FHIR server
- D04: /api/submit/{transactionId} endpoint for PA form submission
Includes 15 new tests covering all functionality.
Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…uplication, and processing queue Implements encounter polling service for athenahealth FHIR integration: - IEncounterPollingService interface extending IHostedService - AthenaPollingService as BackgroundService for polling finished encounters - Deduplication via Dictionary tracking processed encounter IDs - Channel-based processing queue for downstream consumers - AthenaOptions configuration for FHIR API and polling settings Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…d PDF generation Implements the encounter processor that orchestrates prior authorization form generation when encounters are completed. - Add IEncounterProcessor interface for processing finished encounters - Add INotificationHub interface for broadcasting PA form completion events - Implement EncounterProcessor with 5-step pipeline: 1. Hydrate clinical context via FHIR data aggregator 2. Analyze data via Intelligence service 3. Generate PDF via PDF stamper 4. Store PDF in result store with transaction ID 5. Notify subscribers via notification hub - Add comprehensive tests (8 new tests) covering all steps Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
…ests - Resolve AthenaOptions conflict (use Group A version with validation) - Resolve INotificationHub conflict (merge documentation) - Add deprecated AccessToken property for backward compatibility - Fix AthenaPollingServiceTests to include required TokenEndpoint Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Add AddAthenaServices() extension method for DI registration - Register AthenaOptions, AthenaTokenStrategy, AthenaPollingService, EncounterProcessor - Create TokenStrategyResolver for multi-EHR token strategy selection - Wire services in Program.cs Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds Athena integration: configuration, token acquisition and resolver, background encounter polling, encounter processing (PA analysis, PDF stamping, caching), notification hub with SSE streaming, submit endpoint, DI registrations, and extensive unit tests covering these components. Changes
Sequence Diagram(s)sequenceDiagram
participant Athena as Athena FHIR API
participant Poller as AthenaPollingService
participant Processor as EncounterProcessor
participant Aggregator as IFhirDataAggregator
participant Intelligence as IIntelligenceClient
participant Stamper as IPdfFormStamper
participant Store as IAnalysisResultStore
participant Hub as NotificationHub
Poller->>Athena: SearchAsync(status=finished, date filter)
Athena-->>Poller: Bundle[Encounter]
loop for each new encounter
Poller->>Processor: ProcessEncounterAsync(encounterId, patientId)
Processor->>Aggregator: AggregateClinicalDataAsync(encounterId)
Aggregator-->>Processor: ClinicalBundle
Processor->>Intelligence: AnalyzeAsync(ClinicalBundle)
Intelligence-->>Processor: AnalysisResult
Processor->>Stamper: StampFormAsync(PAFormData)
Stamper-->>Processor: PDF bytes
Processor->>Store: SetCachedPdfAsync(key, PDF bytes)
Processor->>Hub: WriteAsync(Notification PA_FORM_READY)
end
sequenceDiagram
participant Client as HTTP Client
participant SSE as SseEndpoints
participant Hub as NotificationHub
participant Reader as Subscriber Reader
Client->>SSE: GET /api/events
SSE->>SSE: Set text/event-stream, no-cache, keep-alive
loop stream
SSE->>Hub: ReadAllAsync(ct)
Hub-->>Reader: Notification
Reader-->>SSE: Notification
SSE->>Client: Write "data: {json}\n\n" and flush
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In `@apps/gateway/Gateway.API.Tests/Services/Http/AthenaTokenStrategyTests.cs`:
- Around line 10-31: The AthenaTokenStrategyTests class creates _mockHandler and
_httpClient but never disposes them; implement IDisposable on the
AthenaTokenStrategyTests class and add a Dispose() method that calls
_httpClient?.Dispose() and _mockHandler?.Dispose() (or equivalent) to release
resources, and ensure the class signature and constructor remain unchanged so
NUnit will invoke Dispose after tests; reference the fields _mockHandler,
_httpClient and the class AthenaTokenStrategyTests when making the change.
In `@apps/gateway/Gateway.API/Contracts/INotificationHub.cs`:
- Around line 7-38: This file contains both the INotificationHub interface and
the public record Notification; split Notification into its own file: create a
new file declaring the public sealed record Notification(...) with the same
namespace and XML docs, remove the record declaration from INotificationHub.cs
leaving only the interface (INotificationHub and its members unchanged), update
any using/imports if necessary and ensure project includes the new file so
consumers referencing Notification continue to compile.
In `@apps/gateway/Gateway.API/DependencyExtensions.cs`:
- Around line 137-140: Replace the call to ValidateDataAnnotations() on the
AddOptions<AthenaOptions>() registration with a predicate-based Validate that
calls AthenaOptions.IsValid() so missing required properties fail at startup;
specifically, update the
services.AddOptions<AthenaOptions>().Bind(...).ValidateDataAnnotations() chain
to use .Validate(options => options.IsValid(), "<clear message>") (keeping the
Bind call) so AthenaOptions.IsValid() is executed during startup validation and
provides a clear failure message when invalid.
In `@apps/gateway/Gateway.API/Endpoints/SubmitEndpoints.cs`:
- Around line 54-74: The endpoint is using transactionId alone while
EncounterProcessor stores cached PDFs with a composite key
"{encounterId}:{transactionId}", causing cache misses; update the key derivation
in this handler to match the writer by combining encounterId and transactionId
before calling resultStore.GetCachedPdfAsync,
resultStore.GetCachedResponseAsync, and resultStore.SetCachedPdfAsync (i.e.,
derive the same "{encounterId}:{transactionId}" key used by EncounterProcessor)
while keeping StampFormAsync usage unchanged.
In `@apps/gateway/Gateway.API/Services/EncounterProcessor.cs`:
- Around line 20-22: Remove the hardcoded DefaultAccessToken constant and
instead obtain the token via the ITokenAcquisitionStrategy (or configured
resolver) used by EncounterProcessor; locate the DefaultAccessToken symbol and
replace its usage with a call to the token acquisition interface (inject
ITokenAcquisitionStrategy into the class constructor or use the existing
resolver property), validate the returned token is non-empty and if missing log
an error and exit early from the processing path (throw or return) to prevent
unauthenticated calls. Ensure all references to DefaultAccessToken are updated
to use the acquired token.
In `@apps/gateway/Gateway.API/Services/Notifications/NotificationHub.cs`:
- Around line 10-37: NotificationHub currently uses a single shared
Channel<Notification> (_channel) so multiple consumers compete for messages;
change to a fan-out model: replace the single _channel with a thread-safe
collection of per-subscriber channels (e.g., ConcurrentDictionary/ConcurrentBag
of Channel<Notification>), update ReadAllAsync to create a new
Channel<Notification> for the caller, add it to the subscriber collection and
return its Reader.ReadAllAsync(ct), and ensure the subscriber channel is
removed/closed when the reader cancels or completes; update WriteAsync to
iterate the subscriber channels and write each notification to every subscriber
(use TryWrite or async WriteAsync per channel and handle/ignore failed writes to
closed channels), and ensure proper cleanup of closed channels to avoid leaks.
In `@apps/gateway/Gateway.API/Services/Polling/AthenaPollingService.cs`:
- Around line 1-7: AthenaPollingService currently uses the obsolete static
AccessToken when calling the FHIR client; replace that by injecting and using
the token acquisition strategy (e.g., ITokenAcquisitionStrategy or similar)
inside the polling flow so a fresh/cached token is obtained before each FHIR
client call. Locate usages of AccessToken in the AthenaPollingService class (the
polling method and any lines around the FHIR client calls referenced in the
review) and replace them with await tokenStrategy.GetTokenAsync(...) or the
equivalent method on the injected strategy; ensure the strategy is injected via
the constructor, used per-poll, and pass the returned token (not AccessToken) to
the FHIR client call. Ensure you remove references to the obsolete AccessToken
and do not store tokens statically in the service.
- Around line 148-164: The poll captures _lastCheck after calling
_fhirClient.SearchAsync which can skip encounters created during the request;
capture a timestamp (e.g., var now = DateTimeOffset.UtcNow) immediately before
calling SearchAsync in PollForFinishedEncountersAsync and, only if the search
succeeds (result.IsFailure is false), assign _lastCheck = now; leave _lastCheck
unchanged on failure so retries won't miss records. Use the existing
_fhirClient.SearchAsync, _lastCheck and PollForFinishedEncountersAsync symbols
to locate and apply this change.
🧹 Nitpick comments (8)
apps/gateway/Gateway.API.Tests/Services/DocumentUploaderTests.cs (2)
103-124: Consider adding a complementary test for null encounterId.This test verifies both references are present when provided, but the tests at lines 75 and 96 pass
nullforencounterIdwithout explicitly asserting thatEncounter/is absent from the payload. Adding a dedicated assertion ensures the conditional linking logic is correctly tested.💡 Suggested additional test
[Test] public async Task DocumentUploader_UploadDocumentAsync_OmitsEncounterReference_WhenEncounterIdIsNull() { // Arrange var pdfBytes = new byte[] { 0x25, 0x50, 0x44, 0x46 }; const string patientId = "patient-123"; const string accessToken = "bearer-token"; string? capturedJson = null; var responseJson = JsonDocument.Parse("{\"id\": \"doc-123\"}").RootElement; _fhirHttpClient.CreateAsync("DocumentReference", Arg.Do<string>(json => capturedJson = json), accessToken, Arg.Any<CancellationToken>()) .Returns(Result<JsonElement>.Success(responseJson)); // Act await _sut.UploadDocumentAsync(pdfBytes, patientId, null, accessToken, CancellationToken.None); // Assert await Assert.That(capturedJson).IsNotNull(); await Assert.That(capturedJson!).Contains($"Patient/{patientId}"); await Assert.That(capturedJson!).DoesNotContain("Encounter/"); }
37-37: Optional: Extract repeated test data to constants.The PDF magic bytes array
{ 0x25, 0x50, 0x44, 0x46 }is duplicated across all six tests. Extracting to a class-level constant improves maintainability.♻️ Suggested refactor
public class DocumentUploaderTests { + private static readonly byte[] PdfMagicBytes = { 0x25, 0x50, 0x44, 0x46 }; + private readonly IFhirHttpClient _fhirHttpClient;Then replace all
new byte[] { 0x25, 0x50, 0x44, 0x46 }occurrences withPdfMagicBytes.Also applies to: 64-64, 86-86, 107-107, 130-130, 149-149
apps/gateway/Gateway.API/Services/Http/TokenStrategyResolver.cs (1)
18-21: Add a null guard for the strategies enumerable.
Fail fast on DI misconfiguration so the error is explicit and localized.🛡️ Suggested guard clause
public TokenStrategyResolver(IEnumerable<ITokenAcquisitionStrategy> strategies) { + if (strategies is null) + { + throw new ArgumentNullException(nameof(strategies)); + } _strategies = strategies; }As per coding guidelines: Guard clauses first, early return, no arrow code.
apps/gateway/Gateway.API/Services/EncounterProcessor.cs (1)
54-60: Add guard clauses for missing encounter/patient IDs.This prevents noisy logs and downstream calls with invalid identifiers. As per coding guidelines: “Control: Guard clauses first, early return, no arrow code.”
🛠️ Suggested fix
public async Task ProcessEncounterAsync(string encounterId, string patientId, CancellationToken ct) { + if (string.IsNullOrWhiteSpace(encounterId) || string.IsNullOrWhiteSpace(patientId)) + { + _logger.LogWarning("EncounterId/PatientId missing; skipping processing."); + return; + } + _logger.LogInformation( "Processing encounter {EncounterId} for patient {PatientId}", encounterId, patientId);apps/gateway/Gateway.API.Tests/Services/Http/AthenaTokenStrategyTests.cs (1)
262-315: Keep only one public type per file.Make helper classes internal (or move them to separate files) so this file exposes a single public type. As per coding guidelines: “SOLID: One public type per file, Strategy over switches, constructor injection.”
🛠️ Suggested fix
-public class FakeTimeProvider : TimeProvider +internal sealed class FakeTimeProvider : TimeProvider @@ -public class MockHttpMessageHandler : HttpMessageHandler +internal sealed class MockHttpMessageHandler : HttpMessageHandlerapps/gateway/Gateway.API/Endpoints/SubmitEndpoints.cs (1)
105-121: Move SubmitRequest to its own file.This keeps a single public type per file and matches the project’s SOLID guideline. As per coding guidelines: “SOLID: One public type per file, Strategy over switches, constructor injection.”
apps/gateway/Gateway.API/Services/Http/AthenaTokenStrategy.cs (1)
13-96: Make token refresh single‑flight and dispose HTTP resources.Concurrent callers can stampede the token endpoint; disposing request/response objects avoids socket pressure.
♻️ Proposed refactor
public sealed class AthenaTokenStrategy : ITokenAcquisitionStrategy { private const int TokenExpiryBufferSeconds = 60; + private readonly SemaphoreSlim _refreshLock = new(1, 1); @@ - var requestContent = new FormUrlEncodedContent(new Dictionary<string, string> - { - ["grant_type"] = "client_credentials", - ["client_id"] = _options.ClientId, - ["client_secret"] = _options.ClientSecret ?? string.Empty - }); - - try - { - var response = await client.PostAsync(_options.TokenEndpoint, requestContent, cancellationToken); + await _refreshLock.WaitAsync(cancellationToken); + try + { + if (IsCachedTokenValid()) + { + return _cachedToken; + } + + using var requestContent = new FormUrlEncodedContent(new Dictionary<string, string> + { + ["grant_type"] = "client_credentials", + ["client_id"] = _options.ClientId, + ["client_secret"] = _options.ClientSecret ?? string.Empty + }); + + using var response = await client.PostAsync(_options.TokenEndpoint, requestContent, cancellationToken); @@ - } - catch - { - return null; - } + } + catch + { + return null; + } + finally + { + _refreshLock.Release(); + }apps/gateway/Gateway.API/Services/Polling/AthenaPollingService.cs (1)
168-205: Validate bundle entries with Firely before enqueueing.Currently any entry with an id is queued; parse/validate the bundle with Firely and ensure resourceType/status are Encounter/finished before enqueueing to avoid incorrect processing. As per coding guidelines, FHIR: Use Firely SDK patterns, validate FHIR resources properly.
apps/gateway/Gateway.API.Tests/Services/Http/AthenaTokenStrategyTests.cs
Outdated
Show resolved
Hide resolved
Fixes: - P2 Major (7 issues): - Split Notification record to own file (SOLID SRP) - Use predicate validation for AthenaOptions.IsValid() - Fix cache key mismatch in SubmitEndpoints (composite key) - Remove hardcoded token, inject ITokenAcquisitionStrategy - Implement fan-out broadcast in NotificationHub - Use TokenStrategyResolver in AthenaPollingService - Capture poll timestamp before SearchAsync - P3 Minor (1 issue): - Add IDisposable to AthenaTokenStrategyTests Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Fix all issues with AI agents
In
`@apps/gateway/Gateway.API.Tests/Services/Polling/AthenaPollingServiceTests.cs`:
- Around line 23-53: Replace the synchronous action-style attributes on the test
lifecycle methods: change [Before(Test)] on Setup() to [SetUp] and change
[After(Test)] on Cleanup() to [TearDown], keeping their async Task signatures
and existing logic (references: Setup(), Cleanup(), and the AthenaPollingService
_sut initialization/teardown); this ensures the async operations (token strategy
setup and _sut.StopAsync/_sut.Dispose) are properly awaited by the NUnit runner.
In `@apps/gateway/Gateway.API/Endpoints/SubmitEndpoints.cs`:
- Around line 107-126: Move the public record SubmitRequest out of the
SubmitEndpoints.cs file into its own file: create a new file named
SubmitRequest.cs under the same namespace Gateway.API.Endpoints, copy the
SubmitRequest record (including XML docs and properties PatientId, EncounterId,
AccessToken) into it, ensure the namespace and accessibility remain the same,
and then remove the SubmitRequest declaration from SubmitEndpoints.cs so that
SubmitEndpoints remains the only public type in its file.
In `@apps/gateway/Gateway.API/Services/EncounterProcessor.cs`:
- Around line 108-114: EncounterProcessor stores only the PDF with
SetCachedPdfAsync using cacheKey, but SubmitEndpoints.SubmitAsync expects a
cached formData via resultStore.GetCachedResponseAsync(cacheKey) for its
fallback; update EncounterProcessor (where SetCachedPdfAsync is called) to also
call resultStore.SetCachedResponseAsync(cacheKey, formData, ct) with the same
cacheKey and the formData object used to build the PDF so the submit endpoint
can retrieve the form data for regeneration.
In `@apps/gateway/Gateway.API/Services/Polling/AthenaPollingService.cs`:
- Around line 110-133: Validate and guard _options.PollingIntervalSeconds before
using it in ExecuteAsync to avoid negative or zero values causing exceptions or
hot loops; if _options.PollingIntervalSeconds <= 0, set a sane default (e.g., 1
second) or use Math.Max(1, _options.PollingIntervalSeconds) when computing the
delay, and log a warning via _logger indicating the value was invalid and
adjusted; apply this check either once at the start of ExecuteAsync or
immediately before the Task.Delay call that currently uses
_options.PollingIntervalSeconds.
🧹 Nitpick comments (7)
apps/gateway/Gateway.API.Tests/Services/Http/AthenaTokenStrategyTests.cs (2)
10-10: Seal the test class to match the default-sealed guideline.
This class isn’t intended for inheritance, so sealing it keeps the surface area tight.Suggested change
-public class AthenaTokenStrategyTests : IDisposable +public sealed class AthenaTokenStrategyTests : IDisposableAs per coding guidelines, “Types: Sealed by default, records for DTOs, nullable enabled.”
268-321: Make helper types non-public, seal them, and avoid arrow members.
This file currently declares multiple public types and uses expression-bodied members, which conflicts with the guidelines.Suggested change
-public class FakeTimeProvider : TimeProvider +internal sealed class FakeTimeProvider : TimeProvider { private DateTimeOffset _utcNow = DateTimeOffset.UtcNow; - public override DateTimeOffset GetUtcNow() => _utcNow; + public override DateTimeOffset GetUtcNow() + { + return _utcNow; + } - public void Advance(TimeSpan duration) => _utcNow = _utcNow.Add(duration); + public void Advance(TimeSpan duration) + { + _utcNow = _utcNow.Add(duration); + } - public void SetTime(DateTimeOffset time) => _utcNow = time; + public void SetTime(DateTimeOffset time) + { + _utcNow = time; + } } -public class MockHttpMessageHandler : HttpMessageHandler +internal sealed class MockHttpMessageHandler : HttpMessageHandler { @@ - public void ResetRequestCount() => RequestCount = 0; + public void ResetRequestCount() + { + RequestCount = 0; + }As per coding guidelines, “SOLID: One public type per file” and “Control: … no arrow code” and “Types: Sealed by default, records for DTOs, nullable enabled.”
apps/gateway/Gateway.API/Services/Notifications/NotificationHub.cs (1)
59-62: Consider completing the channel writer on subscriber removal.When a subscriber is removed (via cancellation or disconnect), the channel's writer is never completed. While the channel will be garbage collected after removal from
_subscribers, explicitly completing the writer signals no more items will arrive—useful if any code path holds a reference.♻️ Optional cleanup enhancement
finally { - _subscribers.TryRemove(id, out _); + if (_subscribers.TryRemove(id, out var removed)) + { + removed.Writer.TryComplete(); + } }apps/gateway/Gateway.API/Services/EncounterProcessor.cs (1)
132-147: Consider notifying subscribers on processing errors.When processing fails, the error is logged but subscribers receive no notification. Dashboard clients waiting for
PA_FORM_READYwon't know the encounter failed. Consider sending aPROCESSING_ERRORnotification.♻️ Suggested error notification
catch (HttpRequestException ex) { _logger.LogError(ex, "Service error for encounter {EncounterId}: {Message}", encounterId, ex.Message); - // Graceful handling - don't propagate, just log + + await _notificationHub.WriteAsync(new Notification( + Type: "PROCESSING_ERROR", + TransactionId: transactionId, + EncounterId: encounterId, + PatientId: patientId, + Message: $"Service error: {ex.Message}"), ct); } catch (Exception ex) { _logger.LogError(ex, "Unexpected error processing encounter {EncounterId}: {Message}", encounterId, ex.Message); - // Graceful handling - don't propagate, just log + + await _notificationHub.WriteAsync(new Notification( + Type: "PROCESSING_ERROR", + TransactionId: transactionId, + EncounterId: encounterId, + PatientId: patientId, + Message: "Unexpected processing error"), ct); }apps/gateway/Gateway.API.Tests/Endpoints/SseEndpointsTests.cs (2)
24-26:Task.Delayintroduces potential test flakiness.Using
Task.Delay(50)to wait for subscriber registration is timing-dependent and could cause intermittent failures under load or on slower CI runners. Consider using a more deterministic synchronization mechanism.♻️ Alternative: expose subscriber count or use synchronization primitive
One approach is to add a test-only hook or expose subscriber count on
NotificationHub:// In NotificationHub (test-visible) internal int SubscriberCount => _subscribers.Count;Then in tests:
// Poll until subscriber is registered (with timeout) var timeout = TimeSpan.FromSeconds(1); var sw = Stopwatch.StartNew(); while (hub.SubscriberCount == 0 && sw.Elapsed < timeout) { await Task.Delay(10); }This is more deterministic than a fixed delay.
Also applies to: 53-55, 92-94, 121-123
101-107: Minor: dispose theStreamReader.The
StreamReaderon line 103 isn't disposed. While not critical in test code, it's good practice.♻️ Suggested fix
// Assert - Check the response body memoryStream.Seek(0, SeekOrigin.Begin); - var streamedContent = new StreamReader(memoryStream).ReadToEnd(); + using var reader = new StreamReader(memoryStream); + var streamedContent = reader.ReadToEnd();apps/gateway/Gateway.API/Services/Polling/AthenaPollingService.cs (1)
187-232: Use Firely SDK to parse and validate FHIR bundles instead of raw JSON.Currently the code accepts any JSON structure without validating that entries contain valid Encounter resources with appropriate status. Using
FhirJsonParserensures type safety, validates the FHIR schema, and prevents accidentally processing non-Encounter resources (e.g., OperationOutcome). TheHl7.Fhir.R4package is already available as a dependency.♻️ Example refactor using Firely parsing
- private async Task ProcessEncounterBundleAsync(JsonElement bundle, CancellationToken ct) - { - if (!bundle.TryGetProperty("entry", out var entries)) - { - return; - } - - foreach (var entry in entries.EnumerateArray()) - { - if (!entry.TryGetProperty("resource", out var resource)) - { - continue; - } - - if (!resource.TryGetProperty("id", out var idElement)) - { - continue; - } - - var encounterId = idElement.GetString(); - if (string.IsNullOrEmpty(encounterId)) - { - continue; - } + private async Task ProcessEncounterBundleAsync(JsonElement bundle, CancellationToken ct) + { + Bundle bundleResource; + try + { + var parser = new FhirJsonParser(); + bundleResource = parser.Parse<Bundle>(bundle.GetRawText()); + } + catch (FormatException ex) + { + _logger.LogWarning(ex, "Invalid FHIR bundle payload."); + return; + } + + if (bundleResource.Entry is null) + { + return; + } + + foreach (var entry in bundleResource.Entry) + { + if (entry.Resource is not Encounter encounter || + encounter.Status != Encounter.EncounterStatus.Finished || + string.IsNullOrWhiteSpace(encounter.Id)) + { + continue; + } + + var encounterId = encounter.Id;Per coding guidelines: FHIR validation should use Firely SDK patterns.
apps/gateway/Gateway.API.Tests/Services/Polling/AthenaPollingServiceTests.cs
Show resolved
Hide resolved
Fixes: - P2 Major (2 issues): - Move SubmitRequest to its own file (SOLID one-type-per-file) - Cache formData alongside PDF in EncounterProcessor for submit fallback - P3 Minor (1 issue): - Guard against non-positive polling interval in AthenaPollingService Note: AthenaPollingServiceTests [Before(Test)]/[After(Test)] attributes are correct - this is TUnit (not NUnit) which fully supports async Task. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@apps/gateway/Gateway.API/Services/EncounterProcessor.cs`:
- Around line 133-148: The catch blocks in EncounterProcessor that currently
only log and swallow exceptions must notify subscribers on failure so clients
aren't left waiting; update both the HttpRequestException and general Exception
handlers to invoke the existing notification/publication mechanism (e.g., call
the service/publisher used for success notifications—refer to the same notifier
used elsewhere in EncounterProcessor such as _notifier or
PublishEncounterResult/PublishEncounterProcessingSucceeded) to emit a failure
event for the encounterId including a safe error message or code (avoid leaking
sensitive stack traces), then log the error as now and continue not rethrowing;
ensure the failure notification contains encounterId and enough context for the
Dashboard to render an error state.
🧹 Nitpick comments (3)
apps/gateway/Gateway.API/Endpoints/SubmitEndpoints.cs (1)
46-57: Consider adding guard clauses for empty/whitespace inputs.The
requiredkeyword ensures non-null at compile time, but empty strings can still slip through at runtime. Adding upfront validation would yield clearer 400 errors instead of opaque downstream failures.🛡️ Optional: Add input validation
public static async Task<IResult> SubmitAsync( string transactionId, SubmitRequest request, IDocumentUploader documentUploader, IAnalysisResultStore resultStore, IPdfFormStamper pdfStamper, CancellationToken ct) { + if (string.IsNullOrWhiteSpace(transactionId)) + { + return Results.BadRequest(new ErrorResponse + { + Message = "Transaction ID is required", + Code = "INVALID_TRANSACTION_ID" + }); + } + + if (string.IsNullOrWhiteSpace(request.PatientId)) + { + return Results.BadRequest(new ErrorResponse + { + Message = "Patient ID is required", + Code = "INVALID_PATIENT_ID" + }); + } + + if (string.IsNullOrWhiteSpace(request.AccessToken)) + { + return Results.BadRequest(new ErrorResponse + { + Message = "Access token is required", + Code = "INVALID_ACCESS_TOKEN" + }); + } + // Derive cache key: composite when encounterId available, otherwise transactionId only var cacheKey = request.EncounterId is nullapps/gateway/Gateway.API/Services/EncounterProcessor.cs (1)
44-51: Consider adding null guards for constructor parameters.Defensive null checks help catch DI misconfiguration early and produce clearer stack traces than NullReferenceExceptions during method execution.
🛡️ Optional: Add argument validation
{ + ArgumentNullException.ThrowIfNull(aggregator); + ArgumentNullException.ThrowIfNull(intelligenceClient); + ArgumentNullException.ThrowIfNull(pdfStamper); + ArgumentNullException.ThrowIfNull(resultStore); + ArgumentNullException.ThrowIfNull(notificationHub); + ArgumentNullException.ThrowIfNull(tokenStrategy); + ArgumentNullException.ThrowIfNull(logger); + _aggregator = aggregator; _intelligenceClient = intelligenceClient; _pdfStamper = pdfStamper; _resultStore = resultStore; _notificationHub = notificationHub; _tokenStrategy = tokenStrategy; _logger = logger; }apps/gateway/Gateway.API/Services/Polling/AthenaPollingService.cs (1)
216-234: Consider removing the redundantisNewvariable.Since line 223 uses
continuewhen the encounter is already processed, execution only reaches lines 226-234 for new encounters. TheisNewvariable will always betrueat line 230, making both the variable and the conditional check unnecessary.♻️ Simplified deduplication logic
// Deduplication: skip if already processed - bool isNew; lock (_lock) { if (_processedEncounters.ContainsKey(encounterId)) { _logger.LogDebug("Skipping already processed encounter: {EncounterId}", encounterId); continue; } _processedEncounters[encounterId] = DateTimeOffset.UtcNow; - isNew = true; } - if (isNew) - { - _logger.LogInformation("Found finished encounter: {EncounterId}", encounterId); - await _encounterChannel.Writer.WriteAsync(encounterId, ct); - } + _logger.LogInformation("Found finished encounter: {EncounterId}", encounterId); + await _encounterChannel.Writer.WriteAsync(encounterId, ct); }
Fixes: - Major: Add error notifications to EncounterProcessor catch blocks - Major: Improve NotificationHub fan-out with TryWrite and cleanup - Minor: Add polling interval guard in AthenaPollingService - Add tests for error notification behavior Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@apps/gateway/Gateway.API.Tests/Services/EncounterProcessorTests.cs`:
- Around line 334-360: The test
ProcessEncounterAsync_AcquiresTokenViaStrategy_PassesToAggregator is currently
using SetupSuccessfulMocks which configures
_aggregator.AggregateClinicalDataAsync to accept Arg.Any<string>() for the
token, masking the assertion; update the test so the mock expectation for
_aggregator.AggregateClinicalDataAsync is configured to require the exact
expectedToken (or move/override the mock setup after calling
SetupSuccessfulMocks) before invoking _sut.ProcessEncounterAsync, ensuring the
assertion that _aggregator.AggregateClinicalDataAsync(patientId, expectedToken,
Arg.Any<CancellationToken>()) actually validates the passed token.
In `@apps/gateway/Gateway.API/Services/EncounterProcessor.cs`:
- Around line 133-147: The HttpRequestException handler currently forwards
ex.Message into the user-facing notification via _notificationHub.WriteAsync
(Notification Type "PROCESSING_ERROR" with TransactionId/EncounterId/PatientId),
which can leak internal details; change the Message sent in
_notificationHub.WriteAsync to a sanitized, non-sensitive string (e.g., a
generic "Service error processing encounter" or a short redact like "Service
error: unavailable") while keeping the full ex and ex.Message in the internal
_logger.LogError call for debugging; update the code in the
catch(HttpRequestException ex) block where _notificationHub.WriteAsync is
invoked so only the sanitized message is sent to clients.
🧹 Nitpick comments (4)
apps/gateway/Gateway.API/Services/Notifications/NotificationHub.cs (1)
24-34: UnusedCancellationTokenparameter.The
ctparameter is accepted per the interface contract but never used. SinceTryWriteis synchronous, there's no await to cancel. If you want to honor cancellation semantics, you could add an early-exit check at method entry. Alternatively, suppress the analyzer warning with a discard or pragma if this is intentional.♻️ Optional: honor cancellation at method entry
public Task WriteAsync(Notification notification, CancellationToken ct) { + ct.ThrowIfCancellationRequested(); + // Fan-out: broadcast to all subscribers using TryWrite for graceful handling of closed channels foreach (var channel in _subscribers.Values) {apps/gateway/Gateway.API/Services/EncounterProcessor.cs (1)
140-146: Error notifications may fail silently if cancellation is already requested.When an exception is caught, the
CancellationToken ctpassed toWriteAsyncmay already be in a cancelled state (e.g., if the caller cancelled the operation). This could prevent the error notification from reaching subscribers.Consider using
CancellationToken.Nonefor error notifications to ensure delivery regardless of the original token's state.Suggested fix
// Notify subscribers of the processing error await _notificationHub.WriteAsync(new Notification( Type: "PROCESSING_ERROR", TransactionId: transactionId, EncounterId: encounterId, PatientId: patientId, - Message: $"Service error: {ex.Message}"), ct); + Message: "Service communication error"), CancellationToken.None); } catch (Exception ex) { _logger.LogError(ex, "Unexpected error processing encounter {EncounterId}: {Message}", encounterId, ex.Message); // Notify subscribers of the processing error (no sensitive stack trace) await _notificationHub.WriteAsync(new Notification( Type: "PROCESSING_ERROR", TransactionId: transactionId, EncounterId: encounterId, PatientId: patientId, - Message: "Unexpected processing error"), ct); + Message: "Unexpected processing error"), CancellationToken.None);Also applies to: 155-161
apps/gateway/Gateway.API.Tests/Services/EncounterProcessorTests.cs (1)
244-265: Missing assertion for formData caching.The test
ProcessEncounterAsync_StoresPdfInResultStore_WithTransactionIdverifies thatSetCachedPdfAsyncis called but doesn't verifySetCachedResponseAsync(formData caching). Since both are critical for the submit fallback path, consider adding a test or extending this one.Suggested addition
// Assert - PDF should be cached with a transaction key await _resultStore.Received(1).SetCachedPdfAsync( Arg.Is<string>(key => key.Contains(encounterId)), pdfBytes, Arg.Any<CancellationToken>()); + + // Assert - FormData should also be cached with the same key + await _resultStore.Received(1).SetCachedResponseAsync( + Arg.Is<string>(key => key.Contains(encounterId)), + formData, + Arg.Any<CancellationToken>());apps/gateway/Gateway.API/Services/Polling/AthenaPollingService.cs (1)
199-242: Validate Encounter resources (or parse with Firely) before enqueue.
Right now any entry with anidis accepted. Consider validatingresourceType(andstatus) to avoid queuing non-Encounter resources, or parse via Firely SDK types for proper FHIR validation.🔧 Minimal validation (if staying with JsonElement)
foreach (var entry in entries.EnumerateArray()) { if (!entry.TryGetProperty("resource", out var resource)) { continue; } + if (!resource.TryGetProperty("resourceType", out var resourceType) || + !string.Equals(resourceType.GetString(), "Encounter", StringComparison.OrdinalIgnoreCase)) + { + continue; + } + + if (resource.TryGetProperty("status", out var status) && + !string.Equals(status.GetString(), "finished", StringComparison.OrdinalIgnoreCase)) + { + continue; + } if (!resource.TryGetProperty("id", out var idElement)) { continue; }As per coding guidelines: FHIR: Use Firely SDK patterns, validate FHIR resources properly.
Contributor
Author
|
@coderabbitai If there are no remaining unaddressed critical/major issues, approve this PR. |
🧩 Analysis chain🏁 Script executed: Length of output: 23262 🏁 Script executed: Length of output: 8089
✅ Security
✅ Thread Safety
✅ Resource Management
✅ Error Handling
✅ Recent Feedback Addressed
✅ Testing
The implementation is production-ready. Approving now. ✅ Actions performedComments resolved and changes approved. |
This was referenced Jan 31, 2026
10 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements athenahealth EHR integration for the Gateway service, enabling automated prior authorization workflow. The system polls for completed encounters, aggregates clinical data via FHIR APIs, generates PA forms through the Intelligence service, and writes completed PDFs back to patient charts.
Changes
Test Plan
Unit tests covering all new components with mocked dependencies. 118 tests passing.
Results: Tests 118 ✓ · Build 0 errors
Design: docs/designs/2026-01-29-athenahealth-pivot-mvp.md