Conversation
Introduces a data service abstraction to decouple GraphQL resolvers from the concrete MockDataService implementation. MockDataService now implements IDataService, enabling future substitution with a live data service. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Adds ApprovePA and DenyPA methods to IDataService and MockDataService, enabling status transitions from waiting_for_insurance to approved/denied. Includes corresponding GraphQL mutations and 4 new TDD tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Extract duplicate session storage token reads from graphqlClient.ts and customFetch.ts into a single auth.ts module with fallback chain (authscript_session -> smart_session). Includes 6 unit tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Extract per-criterion LLM evaluation into standalone evaluate_criterion() function and replace sequential loop with asyncio.gather bounded by a configurable semaphore (LLM_MAX_CONCURRENT, default 4). This reduces multi-criteria evaluation latency from O(n * latency) to O(latency) while preventing LLM provider rate-limit storms. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Add useApprovePARequest and useDenyPARequest mutation hooks with cache invalidation, plus useConnectionStatus hook that polls paStats every 10s to detect gateway connectivity. Includes 4 unit tests. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Move HTTP client creation from per-request to provider __init__ (singleton pattern) to reuse connection pools. Add configurable timeout (30s) and retry (2x) to all OpenAI-compatible providers. Replace bare except with typed handlers: APITimeoutError returns None, RateLimitError propagates, APIError logs and returns None. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…tion Refactor parse_pdf to run synchronous PyMuPDF extraction via run_in_executor to avoid blocking the async event loop. Extract _extract_sync helper that encapsulates temp file lifecycle. Update analyze_with_documents endpoint to parse multiple PDFs concurrently with asyncio.gather. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…e skeleton Updates all GraphQL resolvers to inject IDataService instead of concrete MockDataService. Adds UseMockData configuration toggle (defaults to true) and a LiveDataService skeleton for future live data integration. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
… up ProcessPARequestAsync Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis PR introduces a comprehensive GraphQL-based API layer between the dashboard and gateway, replacing localStorage-backed mock data with server-driven queries and mutations. It adds GraphQL schema definitions, resolvers, model types, service abstractions, frontend hooks, new UI components for PDF generation and progress tracking, and enhanced backend services with improved concurrency control and error handling for LLM integration. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Dashboard Client
participant GQL as GraphQL Client
participant Gateway as Gateway API
participant DataService as IDataService
participant LLM as LLM Service
Client->>GQL: usePARequests()
GQL->>GQL: getAuthToken()
GQL->>Gateway: query GetPARequests (with Bearer token)
Gateway->>DataService: GetPARequests()
DataService-->>Gateway: PARequestModel[]
Gateway-->>GQL: GraphQL response
GQL-->>Client: Cached data via React Query
Client->>GQL: useCreatePARequest mutation
GQL->>Gateway: mutation CreatePARequest
Gateway->>DataService: CreatePARequest(patient, procedure, diagnosis)
DataService-->>Gateway: PARequestModel
Gateway->>LLM: ProcessPARequestAsync (AI evaluation)
LLM->>LLM: evaluate_criterion (with bounded concurrency)
LLM-->>Gateway: confidence + criteria results
DataService-->>Gateway: Updated PARequestModel
Gateway-->>GQL: Mutation response
GQL->>GQL: invalidate(paRequests, paStats)
GQL-->>Client: Success + refetched data
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/gateway/Gateway.API/Services/MockDataService.cs (1)
284-309:⚠️ Potential issue | 🟡 MinorHandle cancellation to avoid leaving requests stuck in
processingstate.If
Task.Delayis cancelled, the request remains in the"processing"status indefinitely with no path to completion or rollback. Wrap the delay in a try-catch to revert the status on cancellation.Suggested fix
- lock (_lock) + string? previousStatus = null; + lock (_lock) { var idx = _paRequests.FindIndex(r => r.Id == id); if (idx < 0) return null; var existing = _paRequests[idx]; + previousStatus = existing.Status; _paRequests[idx] = existing with { Status = "processing", UpdatedAt = DateTime.UtcNow.ToString("O") }; } - await Task.Delay(2000, ct); + try + { + await Task.Delay(2000, ct); + } + catch (OperationCanceledException) + { + lock (_lock) + { + var idx = _paRequests.FindIndex(r => r.Id == id); + if (idx >= 0 && previousStatus is not null) + { + var existing = _paRequests[idx]; + _paRequests[idx] = existing with { Status = previousStatus, UpdatedAt = DateTime.UtcNow.ToString("O") }; + } + } + return null; + }
🤖 Fix all issues with AI agents
In `@apps/dashboard/src/api/graphqlService.ts`:
- Around line 487-511: In both useApprovePARequest and useDenyPARequest add
invalidation for the specific PA request key (QUERY_KEYS.paRequest(id)) in
onSuccess by using the mutation's variables parameter to extract the id (for
useApprovePARequest variables is the id string, for useDenyPARequest variables
is an object with id), then call queryClient.invalidateQueries({ queryKey:
QUERY_KEYS.paRequest(id) }) along with the existing list/stat/activity
invalidations; update onSuccess signatures to accept (_data, variables) and
derive id accordingly.
In `@apps/gateway/Gateway.API/GraphQL/Mutations/Mutation.cs`:
- Around line 78-84: Replace the expression-bodied mutation methods with
block-bodied methods: change ApprovePARequest(string id, [Service] IDataService
dataService) => dataService.ApprovePA(id); to a block that calls
dataService.ApprovePA(id) and returns the result, and similarly change
DenyPARequest(string id, string reason, [Service] IDataService dataService) =>
dataService.DenyPA(id, reason); to a block that invokes dataService.DenyPA(id,
reason) and returns its result; ensure method signatures (ApprovePARequest and
DenyPARequest) and return types remain unchanged and use standard braces and
return statements.
In `@apps/gateway/Gateway.API/GraphQL/Queries/Query.cs`:
- Around line 17-51: Convert each expression-bodied resolver (GetProcedures,
GetMedications, GetProviders, GetPayers, GetDiagnoses, GetPARequests,
GetPARequest, GetPAStats, GetActivity) to a block-bodied method with braces; add
a guard clause at the top validating the [Service] IDataService parameter (throw
new ArgumentNullException(nameof(dataService)) if null) and for GetPARequest
also validate the id (throw new ArgumentException if null or empty) before
calling the corresponding dataService member or method, then return the result.
In `@apps/gateway/Gateway.API/Services/LiveDataService.cs`:
- Around line 18-84: The review requires replacing expression-bodied members
with block-bodied implementations: for each property and method that currently
uses the "=>" throw new NotImplementedException(...) (e.g., Procedures,
Medications, Providers, Payers, Diagnoses, GetPARequests, GetPARequest,
GetPAStats, GetActivity, CreatePARequest, UpdatePARequest,
ProcessPARequestAsync, SubmitPARequest, AddReviewTime, DeletePARequest,
ApprovePA, DenyPA) change the arrow-bodied form to a normal block body with
braces and a single throw new NotImplementedException("...") statement inside
(preserve the existing exception message and member signatures), ensuring async
methods like ProcessPARequestAsync keep their Task<> return type and
CancellationToken parameter; update each member accordingly so no
expression-bodied members remain.
In `@apps/intelligence/src/api/analyze.py`:
- Line 6: The try/except in _safe_parse is redundant because parse_pdf already
catches all errors and returns a string; remove the broad except Exception block
from _safe_parse (or entirely delete the try/except wrapper) so that _safe_parse
simply awaits/returns parse_pdf directly, or if you want defensive layering,
replace the broad catch with documented, specific exceptions that can actually
propagate from parse_pdf—refer to the _safe_parse and parse_pdf functions to
make the change and update or add a brief comment explaining the chosen
approach.
In `@apps/intelligence/src/config.py`:
- Around line 45-48: The LLM performance config fields (llm_max_concurrent,
llm_timeout, llm_max_retries) lack validation and can be set to invalid values;
update their declarations in the Pydantic model to use pydantic.Field with
sensible constraints (e.g., gt=0 for llm_max_concurrent and llm_max_retries,
gt=0.0 for llm_timeout and possibly le=some_max) so invalid environment inputs
are rejected and defaults remain; locate the fields by name in the config class
and replace the plain type/assignment with Field(...) including appropriate
gt/le bounds and helpful descriptions.
In `@apps/intelligence/src/tests/test_evidence_extractor.py`:
- Around line 75-205: Add explicit return type annotations: annotate all async
test coroutines (test_evaluate_criterion_returns_met_evidence_item,
test_evaluate_criterion_parses_not_met,
test_evaluate_criterion_handles_none_response,
test_extract_evidence_calls_criteria_concurrently,
test_extract_evidence_respects_semaphore_limit) with -> None, and annotate
helper LLM coroutines slow_llm and counting_llm with -> str; update their async
def signatures accordingly so the file complies with the apps/intelligence
type-hint rule.
In `@apps/intelligence/src/tests/test_llm_client.py`:
- Around line 1-187: The tests lack explicit return type annotations; update
every test function (test_get_provider_returns_singleton,
test_openai_provider_creates_client_once,
test_chat_completion_uses_provider_singleton,
test_chat_completion_returns_none_on_timeout,
test_chat_completion_raises_on_rate_limit,
test_chat_completion_returns_none_on_api_error, test_get_provider_thread_safe)
and the helper get_provider_thread to include the return type -> None (e.g., def
test_xxx(...) -> None:) to match the codebase convention for complete type
annotations; no other behavior changes are needed.
In `@apps/intelligence/src/tests/test_pdf_parser.py`:
- Around line 1-49: Add missing type annotations: annotate both async test
functions test_parse_pdf_does_not_block_event_loop and
test_parse_pdf_multiple_docs_parallel with return type -> None, and add type
hints to the helper slow_extract as (pdf_bytes: bytes) -> str; update its
signature and any local variables' annotations if necessary so the functions
follow the codebase pattern of complete function type annotations.
🧹 Nitpick comments (5)
apps/dashboard/src/api/__tests__/auth.test.ts (1)
1-37: Well-structured test suite with good edge case coverage.Tests follow the Arrange-Act-Assert pattern with clear, descriptive names. Coverage includes the key scenarios: primary source, fallback, precedence, and various null/invalid cases.
One minor gap: consider adding a test for invalid JSON in
smart_sessionwhenauthscript_sessionis absent, to verify the fallback path's error handling.🧪 Optional: Add fallback invalid JSON test
it('should return null when session has no access_token field', () => { sessionStorage.setItem('authscript_session', JSON.stringify({ user: 'test' })); expect(getAuthToken()).toBeNull(); }); + + it('should return null when smart_session contains invalid JSON', () => { + sessionStorage.setItem('smart_session', 'not-valid-json'); + expect(getAuthToken()).toBeNull(); + }); });apps/dashboard/src/api/customFetch.ts (1)
8-9: Good refactor to centralize token retrieval.The delegation to
getAuthToken()eliminates duplication. The wrapper functiongetStoredAccessTokenis now a single-line delegation—this maintains backward compatibility but could be simplified by callinggetAuthToken()directly at line 104 if no future customization is planned.Also applies to: 143-150
apps/dashboard/src/api/graphqlService.ts (1)
469-485: Mutation constants are not exported, unlike others in this file.
APPROVE_PA_REQUESTandDENY_PA_REQUESTuseconstwithoutexport, while existing mutations likeCREATE_PA_REQUEST(line 140) are exported. For consistency:♻️ Export mutation constants for consistency
-const APPROVE_PA_REQUEST = gql` +export const APPROVE_PA_REQUEST = gql` mutation ApprovePARequest($id: String!) { approvePARequest(id: $id) { ...PARequestFields } } ${PA_REQUEST_FRAGMENT} `; -const DENY_PA_REQUEST = gql` +export const DENY_PA_REQUEST = gql` mutation DenyPARequest($id: String!, $reason: String!) { denyPARequest(id: $id, reason: $reason) { ...PARequestFields } } ${PA_REQUEST_FRAGMENT} `;apps/gateway/Gateway.API/Program.cs (1)
38-47: Fail fast if LiveDataService is enabled before implementation.LiveDataService currently throws NotImplementedException for all members, so a mis-set flag will break runtime requests. Consider a startup guard or environment gate to catch this early.
🔧 Possible guard
var useMockData = builder.Configuration.GetValue<bool>("UseMockData", true); if (useMockData) { builder.Services.AddSingleton<IDataService, MockDataService>(); } else { + if (builder.Environment.IsProduction()) + throw new InvalidOperationException("LiveDataService is not implemented yet. Set UseMockData=true."); builder.Services.AddSingleton<IDataService, LiveDataService>(); }apps/gateway/Gateway.API.Tests/Services/LiveDataServiceTests.cs (1)
14-22: Prefer type-safe interface checks.String comparisons can yield false positives; a type-based check is safer.
✅ Safer reflection check
- var implements = typeof(LiveDataService).GetInterfaces() - .Any(i => i.Name == "IDataService"); + var implements = typeof(IDataService).IsAssignableFrom(typeof(LiveDataService));
Comment on lines
+487
to
+511
| export function useApprovePARequest() { | ||
| const queryClient = useQueryClient(); | ||
| return useMutation({ | ||
| mutationFn: (id: string) => | ||
| graphqlClient.request(APPROVE_PA_REQUEST, { id }), | ||
| onSuccess: () => { | ||
| queryClient.invalidateQueries({ queryKey: QUERY_KEYS.paRequests }); | ||
| queryClient.invalidateQueries({ queryKey: QUERY_KEYS.paStats }); | ||
| queryClient.invalidateQueries({ queryKey: QUERY_KEYS.activity }); | ||
| }, | ||
| }); | ||
| } | ||
|
|
||
| export function useDenyPARequest() { | ||
| const queryClient = useQueryClient(); | ||
| return useMutation({ | ||
| mutationFn: ({ id, reason }: { id: string; reason: string }) => | ||
| graphqlClient.request(DENY_PA_REQUEST, { id, reason }), | ||
| onSuccess: () => { | ||
| queryClient.invalidateQueries({ queryKey: QUERY_KEYS.paRequests }); | ||
| queryClient.invalidateQueries({ queryKey: QUERY_KEYS.paStats }); | ||
| queryClient.invalidateQueries({ queryKey: QUERY_KEYS.activity }); | ||
| }, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Missing invalidation of specific paRequest(id) query on success.
Other mutation hooks like useProcessPARequest (lines 393-399) and useSubmitPARequest (lines 442-448) invalidate the specific QUERY_KEYS.paRequest(id) query on success. These new hooks only invalidate the list queries, which could leave stale data in the cache if a component is viewing the specific request.
🐛 Add specific query invalidation
export function useApprovePARequest() {
const queryClient = useQueryClient();
return useMutation({
mutationFn: (id: string) =>
graphqlClient.request(APPROVE_PA_REQUEST, { id }),
- onSuccess: () => {
+ onSuccess: (_, id) => {
queryClient.invalidateQueries({ queryKey: QUERY_KEYS.paRequests });
+ queryClient.invalidateQueries({ queryKey: QUERY_KEYS.paRequest(id) });
queryClient.invalidateQueries({ queryKey: QUERY_KEYS.paStats });
queryClient.invalidateQueries({ queryKey: QUERY_KEYS.activity });
},
});
}
export function useDenyPARequest() {
const queryClient = useQueryClient();
return useMutation({
mutationFn: ({ id, reason }: { id: string; reason: string }) =>
graphqlClient.request(DENY_PA_REQUEST, { id, reason }),
- onSuccess: () => {
+ onSuccess: (_, { id }) => {
queryClient.invalidateQueries({ queryKey: QUERY_KEYS.paRequests });
+ queryClient.invalidateQueries({ queryKey: QUERY_KEYS.paRequest(id) });
queryClient.invalidateQueries({ queryKey: QUERY_KEYS.paStats });
queryClient.invalidateQueries({ queryKey: QUERY_KEYS.activity });
},
});
}🤖 Prompt for AI Agents
In `@apps/dashboard/src/api/graphqlService.ts` around lines 487 - 511, In both
useApprovePARequest and useDenyPARequest add invalidation for the specific PA
request key (QUERY_KEYS.paRequest(id)) in onSuccess by using the mutation's
variables parameter to extract the id (for useApprovePARequest variables is the
id string, for useDenyPARequest variables is an object with id), then call
queryClient.invalidateQueries({ queryKey: QUERY_KEYS.paRequest(id) }) along with
the existing list/stat/activity invalidations; update onSuccess signatures to
accept (_data, variables) and derive id accordingly.
Comment on lines
+78
to
+84
| /// <summary>Approves a PA request that is waiting for insurance.</summary> | ||
| public PARequestModel? ApprovePARequest(string id, [Service] IDataService dataService) | ||
| => dataService.ApprovePA(id); | ||
|
|
||
| /// <summary>Denies a PA request that is waiting for insurance.</summary> | ||
| public PARequestModel? DenyPARequest(string id, string reason, [Service] IDataService dataService) | ||
| => dataService.DenyPA(id, reason); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "Mutation.cs" | grep -i gatewayRepository: lvlup-sw/authscript
Length of output: 119
🏁 Script executed:
cat -n ./apps/gateway/Gateway.API/GraphQL/Mutations/Mutation.cs | sed -n '70,90p'Repository: lvlup-sw/authscript
Length of output: 806
Switch to block bodies for these mutations.
These methods use expression-bodied members (arrow syntax), which violates the "no arrow code" standard specified in the coding guidelines for this file path.
✅ Block-bodied versions
/// <summary>Approves a PA request that is waiting for insurance.</summary>
public PARequestModel? ApprovePARequest(string id, [Service] IDataService dataService)
- => dataService.ApprovePA(id);
+ {
+ return dataService.ApprovePA(id);
+ }
/// <summary>Denies a PA request that is waiting for insurance.</summary>
public PARequestModel? DenyPARequest(string id, string reason, [Service] IDataService dataService)
- => dataService.DenyPA(id, reason);
+ {
+ return dataService.DenyPA(id, reason);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| /// <summary>Approves a PA request that is waiting for insurance.</summary> | |
| public PARequestModel? ApprovePARequest(string id, [Service] IDataService dataService) | |
| => dataService.ApprovePA(id); | |
| /// <summary>Denies a PA request that is waiting for insurance.</summary> | |
| public PARequestModel? DenyPARequest(string id, string reason, [Service] IDataService dataService) | |
| => dataService.DenyPA(id, reason); | |
| /// <summary>Approves a PA request that is waiting for insurance.</summary> | |
| public PARequestModel? ApprovePARequest(string id, [Service] IDataService dataService) | |
| { | |
| return dataService.ApprovePA(id); | |
| } | |
| /// <summary>Denies a PA request that is waiting for insurance.</summary> | |
| public PARequestModel? DenyPARequest(string id, string reason, [Service] IDataService dataService) | |
| { | |
| return dataService.DenyPA(id, reason); | |
| } |
🤖 Prompt for AI Agents
In `@apps/gateway/Gateway.API/GraphQL/Mutations/Mutation.cs` around lines 78 - 84,
Replace the expression-bodied mutation methods with block-bodied methods: change
ApprovePARequest(string id, [Service] IDataService dataService) =>
dataService.ApprovePA(id); to a block that calls dataService.ApprovePA(id) and
returns the result, and similarly change DenyPARequest(string id, string reason,
[Service] IDataService dataService) => dataService.DenyPA(id, reason); to a
block that invokes dataService.DenyPA(id, reason) and returns its result; ensure
method signatures (ApprovePARequest and DenyPARequest) and return types remain
unchanged and use standard braces and return statements.
Comment on lines
+17
to
+51
| /// <summary>Gets all available procedures.</summary> | ||
| public IReadOnlyList<ProcedureModel> GetProcedures([Service] IDataService dataService) => | ||
| dataService.Procedures; | ||
|
|
||
| public IReadOnlyList<MedicationModel> GetMedications([Service] MockDataService mockData) => | ||
| mockData.Medications; | ||
| /// <summary>Gets all available medications.</summary> | ||
| public IReadOnlyList<MedicationModel> GetMedications([Service] IDataService dataService) => | ||
| dataService.Medications; | ||
|
|
||
| public IReadOnlyList<ProviderModel> GetProviders([Service] MockDataService mockData) => | ||
| mockData.Providers; | ||
| /// <summary>Gets all available providers.</summary> | ||
| public IReadOnlyList<ProviderModel> GetProviders([Service] IDataService dataService) => | ||
| dataService.Providers; | ||
|
|
||
| public IReadOnlyList<PayerModel> GetPayers([Service] MockDataService mockData) => | ||
| mockData.Payers; | ||
| /// <summary>Gets all available payers.</summary> | ||
| public IReadOnlyList<PayerModel> GetPayers([Service] IDataService dataService) => | ||
| dataService.Payers; | ||
|
|
||
| public IReadOnlyList<DiagnosisModel> GetDiagnoses([Service] MockDataService mockData) => | ||
| mockData.Diagnoses; | ||
| /// <summary>Gets all available diagnoses.</summary> | ||
| public IReadOnlyList<DiagnosisModel> GetDiagnoses([Service] IDataService dataService) => | ||
| dataService.Diagnoses; | ||
|
|
||
| public IReadOnlyList<PARequestModel> GetPARequests([Service] MockDataService mockData) => | ||
| mockData.GetPARequests(); | ||
| /// <summary>Gets all PA requests.</summary> | ||
| public IReadOnlyList<PARequestModel> GetPARequests([Service] IDataService dataService) => | ||
| dataService.GetPARequests(); | ||
|
|
||
| public PARequestModel? GetPARequest(string id, [Service] MockDataService mockData) => | ||
| mockData.GetPARequest(id); | ||
| /// <summary>Gets a single PA request by ID.</summary> | ||
| public PARequestModel? GetPARequest(string id, [Service] IDataService dataService) => | ||
| dataService.GetPARequest(id); | ||
|
|
||
| public PAStatsModel GetPAStats([Service] MockDataService mockData) => | ||
| mockData.GetPAStats(); | ||
| /// <summary>Gets aggregated PA statistics.</summary> | ||
| public PAStatsModel GetPAStats([Service] IDataService dataService) => | ||
| dataService.GetPAStats(); | ||
|
|
||
| public IReadOnlyList<ActivityItemModel> GetActivity([Service] MockDataService mockData) => | ||
| mockData.GetActivity(); | ||
| /// <summary>Gets recent activity items.</summary> | ||
| public IReadOnlyList<ActivityItemModel> GetActivity([Service] IDataService dataService) => | ||
| dataService.GetActivity(); |
There was a problem hiding this comment.
Replace expression-bodied members to comply with “no arrow code.”
The resolver methods are all expression-bodied, which violates the guideline.
🔧 Proposed change (block-bodied methods)
/// <summary>Gets all available procedures.</summary>
- public IReadOnlyList<ProcedureModel> GetProcedures([Service] IDataService dataService) =>
- dataService.Procedures;
+ public IReadOnlyList<ProcedureModel> GetProcedures([Service] IDataService dataService)
+ {
+ return dataService.Procedures;
+ }
/// <summary>Gets all available medications.</summary>
- public IReadOnlyList<MedicationModel> GetMedications([Service] IDataService dataService) =>
- dataService.Medications;
+ public IReadOnlyList<MedicationModel> GetMedications([Service] IDataService dataService)
+ {
+ return dataService.Medications;
+ }
/// <summary>Gets all available providers.</summary>
- public IReadOnlyList<ProviderModel> GetProviders([Service] IDataService dataService) =>
- dataService.Providers;
+ public IReadOnlyList<ProviderModel> GetProviders([Service] IDataService dataService)
+ {
+ return dataService.Providers;
+ }
/// <summary>Gets all available payers.</summary>
- public IReadOnlyList<PayerModel> GetPayers([Service] IDataService dataService) =>
- dataService.Payers;
+ public IReadOnlyList<PayerModel> GetPayers([Service] IDataService dataService)
+ {
+ return dataService.Payers;
+ }
/// <summary>Gets all available diagnoses.</summary>
- public IReadOnlyList<DiagnosisModel> GetDiagnoses([Service] IDataService dataService) =>
- dataService.Diagnoses;
+ public IReadOnlyList<DiagnosisModel> GetDiagnoses([Service] IDataService dataService)
+ {
+ return dataService.Diagnoses;
+ }
/// <summary>Gets all PA requests.</summary>
- public IReadOnlyList<PARequestModel> GetPARequests([Service] IDataService dataService) =>
- dataService.GetPARequests();
+ public IReadOnlyList<PARequestModel> GetPARequests([Service] IDataService dataService)
+ {
+ return dataService.GetPARequests();
+ }
/// <summary>Gets a single PA request by ID.</summary>
- public PARequestModel? GetPARequest(string id, [Service] IDataService dataService) =>
- dataService.GetPARequest(id);
+ public PARequestModel? GetPARequest(string id, [Service] IDataService dataService)
+ {
+ return dataService.GetPARequest(id);
+ }
/// <summary>Gets aggregated PA statistics.</summary>
- public PAStatsModel GetPAStats([Service] IDataService dataService) =>
- dataService.GetPAStats();
+ public PAStatsModel GetPAStats([Service] IDataService dataService)
+ {
+ return dataService.GetPAStats();
+ }
/// <summary>Gets recent activity items.</summary>
- public IReadOnlyList<ActivityItemModel> GetActivity([Service] IDataService dataService) =>
- dataService.GetActivity();
+ public IReadOnlyList<ActivityItemModel> GetActivity([Service] IDataService dataService)
+ {
+ return dataService.GetActivity();
+ }As per coding guidelines, “Control: Guard clauses first, early return, no arrow code.”
🤖 Prompt for AI Agents
In `@apps/gateway/Gateway.API/GraphQL/Queries/Query.cs` around lines 17 - 51,
Convert each expression-bodied resolver (GetProcedures, GetMedications,
GetProviders, GetPayers, GetDiagnoses, GetPARequests, GetPARequest, GetPAStats,
GetActivity) to a block-bodied method with braces; add a guard clause at the top
validating the [Service] IDataService parameter (throw new
ArgumentNullException(nameof(dataService)) if null) and for GetPARequest also
validate the id (throw new ArgumentException if null or empty) before calling
the corresponding dataService member or method, then return the result.
Comment on lines
+18
to
+84
| /// <inheritdoc /> | ||
| public IReadOnlyList<ProcedureModel> Procedures => | ||
| throw new NotImplementedException("LiveDataService.Procedures not yet implemented."); | ||
|
|
||
| /// <inheritdoc /> | ||
| public IReadOnlyList<MedicationModel> Medications => | ||
| throw new NotImplementedException("LiveDataService.Medications not yet implemented."); | ||
|
|
||
| /// <inheritdoc /> | ||
| public IReadOnlyList<ProviderModel> Providers => | ||
| throw new NotImplementedException("LiveDataService.Providers not yet implemented."); | ||
|
|
||
| /// <inheritdoc /> | ||
| public IReadOnlyList<PayerModel> Payers => | ||
| throw new NotImplementedException("LiveDataService.Payers not yet implemented."); | ||
|
|
||
| /// <inheritdoc /> | ||
| public IReadOnlyList<DiagnosisModel> Diagnoses => | ||
| throw new NotImplementedException("LiveDataService.Diagnoses not yet implemented."); | ||
|
|
||
| /// <inheritdoc /> | ||
| public IReadOnlyList<PARequestModel> GetPARequests() => | ||
| throw new NotImplementedException("LiveDataService.GetPARequests not yet implemented."); | ||
|
|
||
| /// <inheritdoc /> | ||
| public PARequestModel? GetPARequest(string id) => | ||
| throw new NotImplementedException("LiveDataService.GetPARequest not yet implemented."); | ||
|
|
||
| /// <inheritdoc /> | ||
| public PAStatsModel GetPAStats() => | ||
| throw new NotImplementedException("LiveDataService.GetPAStats not yet implemented."); | ||
|
|
||
| /// <inheritdoc /> | ||
| public IReadOnlyList<ActivityItemModel> GetActivity() => | ||
| throw new NotImplementedException("LiveDataService.GetActivity not yet implemented."); | ||
|
|
||
| /// <inheritdoc /> | ||
| public PARequestModel CreatePARequest(PatientModel patient, string procedureCode, string diagnosisCode, string diagnosisName, string providerId = "DR001") => | ||
| throw new NotImplementedException("LiveDataService.CreatePARequest not yet implemented."); | ||
|
|
||
| /// <inheritdoc /> | ||
| public PARequestModel? UpdatePARequest(string id, string? diagnosis, string? diagnosisCode, string? serviceDate, string? placeOfService, string? clinicalSummary, IReadOnlyList<CriterionModel>? criteria) => | ||
| throw new NotImplementedException("LiveDataService.UpdatePARequest not yet implemented."); | ||
|
|
||
| /// <inheritdoc /> | ||
| public Task<PARequestModel?> ProcessPARequestAsync(string id, CancellationToken ct = default) => | ||
| throw new NotImplementedException("LiveDataService.ProcessPARequestAsync not yet implemented."); | ||
|
|
||
| /// <inheritdoc /> | ||
| public PARequestModel? SubmitPARequest(string id, int addReviewTimeSeconds = 0) => | ||
| throw new NotImplementedException("LiveDataService.SubmitPARequest not yet implemented."); | ||
|
|
||
| /// <inheritdoc /> | ||
| public PARequestModel? AddReviewTime(string id, int seconds) => | ||
| throw new NotImplementedException("LiveDataService.AddReviewTime not yet implemented."); | ||
|
|
||
| /// <inheritdoc /> | ||
| public bool DeletePARequest(string id) => | ||
| throw new NotImplementedException("LiveDataService.DeletePARequest not yet implemented."); | ||
|
|
||
| /// <inheritdoc /> | ||
| public PARequestModel? ApprovePA(string id) => | ||
| throw new NotImplementedException("LiveDataService.ApprovePA not yet implemented."); | ||
|
|
||
| /// <inheritdoc /> | ||
| public PARequestModel? DenyPA(string id, string reason) => | ||
| throw new NotImplementedException("LiveDataService.DenyPA not yet implemented."); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the LiveDataService.cs file
fd -t f "LiveDataService.cs" apps/gateway
# Check file size and content around lines 18-84
if [ -f "apps/gateway/Gateway.API/Services/LiveDataService.cs" ]; then
echo "=== File found, checking content ==="
wc -l apps/gateway/Gateway.API/Services/LiveDataService.cs
echo "=== Lines 15-90 (to see context) ==="
sed -n '15,90p' apps/gateway/Gateway.API/Services/LiveDataService.cs | cat -n
fiRepository: lvlup-sw/authscript
Length of output: 4116
Convert stubs to block-bodied members.
This file matches the apps/gateway/**/*.cs pattern, which requires "no arrow code." These expression-bodied members should be converted to block bodies.
🔧 Pattern to apply across members
- public IReadOnlyList<ProcedureModel> Procedures =>
- throw new NotImplementedException("LiveDataService.Procedures not yet implemented.");
+ public IReadOnlyList<ProcedureModel> Procedures
+ {
+ get
+ {
+ throw new NotImplementedException("LiveDataService.Procedures not yet implemented.");
+ }
+ }
- public PARequestModel? ApprovePA(string id) =>
- throw new NotImplementedException("LiveDataService.ApprovePA not yet implemented.");
+ public PARequestModel? ApprovePA(string id)
+ {
+ throw new NotImplementedException("LiveDataService.ApprovePA not yet implemented.");
+ }🤖 Prompt for AI Agents
In `@apps/gateway/Gateway.API/Services/LiveDataService.cs` around lines 18 - 84,
The review requires replacing expression-bodied members with block-bodied
implementations: for each property and method that currently uses the "=>" throw
new NotImplementedException(...) (e.g., Procedures, Medications, Providers,
Payers, Diagnoses, GetPARequests, GetPARequest, GetPAStats, GetActivity,
CreatePARequest, UpdatePARequest, ProcessPARequestAsync, SubmitPARequest,
AddReviewTime, DeletePARequest, ApprovePA, DenyPA) change the arrow-bodied form
to a normal block body with braces and a single throw new
NotImplementedException("...") statement inside (preserve the existing exception
message and member signatures), ensuring async methods like
ProcessPARequestAsync keep their Task<> return type and CancellationToken
parameter; update each member accordingly so no expression-bodied members
remain.
| Uses LLM-powered reasoning to extract evidence and generate PA form responses. | ||
| """ | ||
|
|
||
| import asyncio |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -path "*/apps/intelligence/src/api/analyze.py" -type fRepository: lvlup-sw/authscript
Length of output: 101
🏁 Script executed:
cat -n ./apps/intelligence/src/api/analyze.pyRepository: lvlup-sw/authscript
Length of output: 6215
🏁 Script executed:
find . -path "*/src/parsers/pdf_parser.py" -type fRepository: lvlup-sw/authscript
Length of output: 108
🏁 Script executed:
cat -n ./apps/intelligence/src/parsers/pdf_parser.pyRepository: lvlup-sw/authscript
Length of output: 2754
Remove or refactor the redundant exception handling in _safe_parse.
The broad except Exception at line 106 violates the specific-exception guideline. However, parse_pdf already handles all exceptions internally and never raises to its caller—it always returns a string. The proposed fix to catch specific exceptions like OSError or RuntimeError won't work because those exceptions won't reach this code.
Either remove the try-except entirely (since parse_pdf is fault-tolerant), or if defensive layering is intentional, document why and catch only exceptions that can actually propagate from the underlying PDF operations.
🤖 Prompt for AI Agents
In `@apps/intelligence/src/api/analyze.py` at line 6, The try/except in
_safe_parse is redundant because parse_pdf already catches all errors and
returns a string; remove the broad except Exception block from _safe_parse (or
entirely delete the try/except wrapper) so that _safe_parse simply
awaits/returns parse_pdf directly, or if you want defensive layering, replace
the broad catch with documented, specific exceptions that can actually propagate
from parse_pdf—refer to the _safe_parse and parse_pdf functions to make the
change and update or add a brief comment explaining the chosen approach.
Comment on lines
+75
to
+205
| # --- A1: evaluate_criterion tests --- | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_evaluate_criterion_returns_met_evidence_item(): | ||
| """Test that evaluate_criterion returns an EvidenceItem with MET status.""" | ||
| criterion = {"id": "crit-1", "description": "Patient has documented symptoms", "required": True} | ||
| clinical_summary = "Patient presents with chronic lower back pain for 8 weeks." | ||
|
|
||
| mock_llm = AsyncMock(return_value="Based on the clinical data, this criterion is MET. The patient has documented symptoms of chronic lower back pain.") | ||
| with patch("src.reasoning.evidence_extractor.chat_completion", mock_llm): | ||
| result = await evaluate_criterion(criterion, clinical_summary) | ||
|
|
||
| assert isinstance(result, EvidenceItem) | ||
| assert result.criterion_id == "crit-1" | ||
| assert result.status == "MET" | ||
| assert result.confidence == 0.8 | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_evaluate_criterion_parses_not_met(): | ||
| """Test that evaluate_criterion correctly parses NOT_MET response.""" | ||
| criterion = {"id": "crit-2", "description": "Conservative therapy completed", "required": True} | ||
| clinical_summary = "Patient has not attempted physical therapy." | ||
|
|
||
| mock_llm = AsyncMock(return_value="This criterion is NOT MET. No evidence of conservative therapy.") | ||
| with patch("src.reasoning.evidence_extractor.chat_completion", mock_llm): | ||
| result = await evaluate_criterion(criterion, clinical_summary) | ||
|
|
||
| assert result.status == "NOT_MET" | ||
| assert result.confidence == 0.8 | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_evaluate_criterion_handles_none_response(): | ||
| """Test that evaluate_criterion handles LLM returning None gracefully.""" | ||
| criterion = {"id": "crit-3", "description": "Valid diagnosis", "required": False} | ||
| clinical_summary = "Patient data." | ||
|
|
||
| mock_llm = AsyncMock(return_value=None) | ||
| with patch("src.reasoning.evidence_extractor.chat_completion", mock_llm): | ||
| result = await evaluate_criterion(criterion, clinical_summary) | ||
|
|
||
| assert result.status == "UNCLEAR" | ||
| assert result.confidence == 0.5 | ||
|
|
||
|
|
||
| # --- A2: Parallel evidence extraction tests --- | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_extract_evidence_calls_criteria_concurrently(): | ||
| """Test that evidence extraction runs criteria evaluation in parallel.""" | ||
| import asyncio | ||
| import time | ||
|
|
||
| call_times: list[float] = [] | ||
|
|
||
| async def slow_llm(*args, **kwargs): | ||
| call_times.append(time.monotonic()) | ||
| await asyncio.sleep(0.1) # Simulate LLM latency | ||
| return "The criterion is MET based on clinical data." | ||
|
|
||
| bundle = ClinicalBundle( | ||
| patient_id="test-patient", | ||
| patient=PatientInfo(name="Test Patient", birth_date=date(1990, 1, 1)), | ||
| conditions=[Condition(code="M54.5", display="Low back pain")], | ||
| ) | ||
| policy = { | ||
| "name": "Test Policy", | ||
| "criteria": [ | ||
| {"id": f"crit-{i}", "description": f"Criterion {i}", "required": True} | ||
| for i in range(3) | ||
| ], | ||
| "procedure_codes": ["72148"], | ||
| } | ||
|
|
||
| mock_llm = AsyncMock(side_effect=slow_llm) | ||
| with patch("src.reasoning.evidence_extractor.chat_completion", mock_llm): | ||
| start = time.monotonic() | ||
| results = await extract_evidence(bundle, policy) | ||
| duration = time.monotonic() - start | ||
|
|
||
| assert len(results) == 3 | ||
| # If parallel: ~0.1s. If sequential: ~0.3s. Allow margin. | ||
| assert duration < 0.25, f"Expected parallel execution (<0.25s), got {duration:.2f}s" | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_extract_evidence_respects_semaphore_limit(): | ||
| """Test that concurrent LLM calls are bounded by semaphore.""" | ||
| import asyncio | ||
|
|
||
| max_concurrent = 0 | ||
| current_concurrent = 0 | ||
| lock = asyncio.Lock() | ||
|
|
||
| async def counting_llm(*args, **kwargs): | ||
| nonlocal max_concurrent, current_concurrent | ||
| async with lock: | ||
| current_concurrent += 1 | ||
| max_concurrent = max(max_concurrent, current_concurrent) | ||
| await asyncio.sleep(0.05) | ||
| async with lock: | ||
| current_concurrent -= 1 | ||
| return "The criterion is MET." | ||
|
|
||
| bundle = ClinicalBundle( | ||
| patient_id="test-patient", | ||
| patient=PatientInfo(name="Test", birth_date=date(1990, 1, 1)), | ||
| conditions=[Condition(code="M54.5", display="Low back pain")], | ||
| ) | ||
| policy = { | ||
| "name": "Test", | ||
| "criteria": [ | ||
| {"id": f"crit-{i}", "description": f"Criterion {i}", "required": True} | ||
| for i in range(6) | ||
| ], | ||
| "procedure_codes": ["72148"], | ||
| } | ||
|
|
||
| mock_llm = AsyncMock(side_effect=counting_llm) | ||
| with ( | ||
| patch("src.reasoning.evidence_extractor.chat_completion", mock_llm), | ||
| patch("src.reasoning.evidence_extractor._get_llm_semaphore", return_value=asyncio.Semaphore(2)), | ||
| ): | ||
| results = await extract_evidence(bundle, policy) | ||
|
|
||
| assert len(results) == 6 | ||
| assert max_concurrent <= 2, f"Expected max 2 concurrent, got {max_concurrent}" | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if file exists and examine the actual function signatures
if [ -f "apps/intelligence/src/tests/test_evidence_extractor.py" ]; then
echo "File found. Checking function signatures around lines 75-205:"
sed -n '75,205p' "apps/intelligence/src/tests/test_evidence_extractor.py" | grep -E "^\s*(async\s+)?def\s+\w+.*:\s*$" -A 1
else
# Try alternative paths
find . -name "test_evidence_extractor.py" -type f 2>/dev/null | head -5
fiRepository: lvlup-sw/authscript
Length of output: 998
🏁 Script executed:
#!/bin/bash
# Search for the test file with flexible path matching
fd "test_evidence_extractor" --type f 2>/dev/null | head -5Repository: lvlup-sw/authscript
Length of output: 117
🏁 Script executed:
#!/bin/bash
# Get exact function signatures with line numbers to confirm completeness
sed -n '75,205p' "apps/intelligence/src/tests/test_evidence_extractor.py" | grep -n "def " | head -20Repository: lvlup-sw/authscript
Length of output: 465
🏁 Script executed:
#!/bin/bash
# Extract the exact lines around the function definitions to verify annotations
sed -n '78,80p; 85,87p; 93,95p; 103,105p; 106,108p; 120,122p; 123,125p' "apps/intelligence/src/tests/test_evidence_extractor.py"Repository: lvlup-sw/authscript
Length of output: 622
🏁 Script executed:
#!/bin/bash
# Get the complete function signatures to confirm they lack return type annotations
sed -n '78p; 85p; 93p; 103p; 106p; 120p; 123p' "apps/intelligence/src/tests/test_evidence_extractor.py"Repository: lvlup-sw/authscript
Length of output: 166
🏁 Script executed:
#!/bin/bash
# Check the context around slow_llm and counting_llm to verify their return types
sed -n '106,115p; 123,133p' "apps/intelligence/src/tests/test_evidence_extractor.py"Repository: lvlup-sw/authscript
Length of output: 745
🏁 Script executed:
#!/bin/bash
# Extract exact function definitions from the file
cat -n "apps/intelligence/src/tests/test_evidence_extractor.py" | sed -n '78,180p' | grep -E "^\s*[0-9]+\s+(async\s+)?def\s+" | head -20Repository: lvlup-sw/authscript
Length of output: 494
🏁 Script executed:
#!/bin/bash
# Get the full signatures by reading specific line ranges
python3 << 'EOF'
with open("apps/intelligence/src/tests/test_evidence_extractor.py") as f:
lines = f.readlines()
# Check lines 75-205 for function definitions
for i in range(74, min(205, len(lines))):
line = lines[i]
if line.strip().startswith('async def ') or line.strip().startswith('def '):
# Print the function definition line and check if it has return annotation
print(f"Line {i+1}: {line.rstrip()}")
if '->' not in line:
print(" ✗ Missing return type annotation")
else:
print(" ✓ Has return type annotation")
EOFRepository: lvlup-sw/authscript
Length of output: 758
🏁 Script executed:
#!/bin/bash
# Verify the context of return statements in helper functions
python3 << 'EOF'
with open("apps/intelligence/src/tests/test_evidence_extractor.py") as f:
lines = f.readlines()
# Check slow_llm return type
print("=== slow_llm context ===")
for i in range(132, min(145, len(lines))):
print(f"{i+1}: {lines[i].rstrip()}")
print("\n=== counting_llm context ===")
for i in range(171, min(184, len(lines))):
print(f"{i+1}: {lines[i].rstrip()}")
EOFRepository: lvlup-sw/authscript
Length of output: 1237
Add return type annotations for async test functions and helpers.
All test functions and helper coroutines in this section lack return type annotations, violating the type-hint requirement for apps/intelligence/**/*.py. Test functions should return -> None, while slow_llm and counting_llm should return -> str (both return string literals).
Example annotations
-async def test_evaluate_criterion_returns_met_evidence_item():
+async def test_evaluate_criterion_returns_met_evidence_item() -> None:
-async def test_evaluate_criterion_parses_not_met():
+async def test_evaluate_criterion_parses_not_met() -> None:
-async def test_evaluate_criterion_handles_none_response():
+async def test_evaluate_criterion_handles_none_response() -> None:
-async def test_extract_evidence_calls_criteria_concurrently():
+async def test_extract_evidence_calls_criteria_concurrently() -> None:
- async def slow_llm(*args, **kwargs):
+ async def slow_llm(*args, **kwargs) -> str:
-async def test_extract_evidence_respects_semaphore_limit():
+async def test_extract_evidence_respects_semaphore_limit() -> None:
- async def counting_llm(*args, **kwargs):
+ async def counting_llm(*args, **kwargs) -> str:🤖 Prompt for AI Agents
In `@apps/intelligence/src/tests/test_evidence_extractor.py` around lines 75 -
205, Add explicit return type annotations: annotate all async test coroutines
(test_evaluate_criterion_returns_met_evidence_item,
test_evaluate_criterion_parses_not_met,
test_evaluate_criterion_handles_none_response,
test_extract_evidence_calls_criteria_concurrently,
test_extract_evidence_respects_semaphore_limit) with -> None, and annotate
helper LLM coroutines slow_llm and counting_llm with -> str; update their async
def signatures accordingly so the file complies with the apps/intelligence
type-hint rule.
Comment on lines
+1
to
+187
| """Tests for LLM client singleton pooling, timeout, and retry.""" | ||
|
|
||
| import pytest | ||
| from unittest.mock import patch, MagicMock, AsyncMock | ||
|
|
||
|
|
||
| def test_get_provider_returns_singleton(): | ||
| """Test that _get_provider returns the same instance on repeated calls.""" | ||
| import src.llm_client as llm_mod | ||
|
|
||
| # Reset cached provider | ||
| llm_mod._cached_provider = None | ||
|
|
||
| with patch.object(llm_mod, "settings") as mock_settings: | ||
| mock_settings.llm_configured = True | ||
| mock_settings.llm_provider = "openai" | ||
| mock_settings.openai_api_key = "test-key" | ||
| mock_settings.openai_org_id = "" | ||
| mock_settings.openai_model = "gpt-4.1" | ||
| mock_settings.llm_timeout = 30.0 | ||
| mock_settings.llm_max_retries = 2 | ||
|
|
||
| provider1 = llm_mod._get_provider() | ||
| provider2 = llm_mod._get_provider() | ||
|
|
||
| assert provider1 is provider2, "Expected same provider instance (singleton)" | ||
|
|
||
| # Cleanup | ||
| llm_mod._cached_provider = None | ||
|
|
||
|
|
||
| def test_openai_provider_creates_client_once(): | ||
| """Test that OpenAIProvider creates the client in __init__, not per call.""" | ||
| from src.llm_client import OpenAIProvider | ||
|
|
||
| with patch("src.llm_client.settings") as mock_settings: | ||
| mock_settings.openai_api_key = "test-key" | ||
| mock_settings.openai_org_id = "" | ||
| mock_settings.openai_model = "gpt-4.1" | ||
| mock_settings.llm_timeout = 30.0 | ||
| mock_settings.llm_max_retries = 2 | ||
|
|
||
| provider = OpenAIProvider() | ||
|
|
||
| assert hasattr(provider, "_client"), "Provider should store client as _client" | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_chat_completion_uses_provider_singleton(): | ||
| """Test that repeated chat_completion calls reuse the same provider.""" | ||
| import src.llm_client as llm_mod | ||
|
|
||
| llm_mod._cached_provider = None | ||
|
|
||
| # Mock the provider's complete method | ||
| mock_provider = AsyncMock() | ||
| mock_provider.complete = AsyncMock(return_value="test response") | ||
| llm_mod._cached_provider = mock_provider | ||
|
|
||
| with patch.object(llm_mod, "settings") as mock_settings: | ||
| mock_settings.llm_configured = True | ||
|
|
||
| await llm_mod.chat_completion("system", "user") | ||
| await llm_mod.chat_completion("system", "user") | ||
|
|
||
| # Provider should be reused, not re-created | ||
| assert mock_provider.complete.call_count == 2 | ||
|
|
||
| # Cleanup | ||
| llm_mod._cached_provider = None | ||
|
|
||
|
|
||
| # --- A4: Structured error handling tests --- | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_chat_completion_returns_none_on_timeout(): | ||
| """Test that APITimeoutError is caught and returns None.""" | ||
| import src.llm_client as llm_mod | ||
| from openai import APITimeoutError | ||
|
|
||
| mock_provider = AsyncMock() | ||
| mock_provider.complete = AsyncMock(side_effect=APITimeoutError(request=MagicMock())) | ||
| llm_mod._cached_provider = mock_provider | ||
|
|
||
| with patch.object(llm_mod, "settings") as mock_settings: | ||
| mock_settings.llm_configured = True | ||
| result = await llm_mod.chat_completion("system", "user") | ||
|
|
||
| assert result is None | ||
|
|
||
| # Cleanup | ||
| llm_mod._cached_provider = None | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_chat_completion_raises_on_rate_limit(): | ||
| """Test that RateLimitError propagates instead of being swallowed.""" | ||
| import src.llm_client as llm_mod | ||
| from openai import RateLimitError | ||
|
|
||
| mock_provider = AsyncMock() | ||
| mock_response = MagicMock() | ||
| mock_response.status_code = 429 | ||
| mock_response.headers = {} | ||
| mock_provider.complete = AsyncMock( | ||
| side_effect=RateLimitError( | ||
| message="Rate limited", | ||
| response=mock_response, | ||
| body=None, | ||
| ) | ||
| ) | ||
| llm_mod._cached_provider = mock_provider | ||
|
|
||
| with ( | ||
| patch.object(llm_mod, "settings") as mock_settings, | ||
| pytest.raises(RateLimitError), | ||
| ): | ||
| mock_settings.llm_configured = True | ||
| await llm_mod.chat_completion("system", "user") | ||
|
|
||
| # Cleanup | ||
| llm_mod._cached_provider = None | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_chat_completion_returns_none_on_api_error(): | ||
| """Test that APIError is caught, logged, and returns None.""" | ||
| import src.llm_client as llm_mod | ||
| from openai import APIError | ||
|
|
||
| mock_provider = AsyncMock() | ||
| mock_provider.complete = AsyncMock( | ||
| side_effect=APIError(message="Server error", request=MagicMock(), body=None) | ||
| ) | ||
| llm_mod._cached_provider = mock_provider | ||
|
|
||
| with patch.object(llm_mod, "settings") as mock_settings: | ||
| mock_settings.llm_configured = True | ||
| result = await llm_mod.chat_completion("system", "user") | ||
|
|
||
| assert result is None | ||
|
|
||
| # Cleanup | ||
| llm_mod._cached_provider = None | ||
|
|
||
|
|
||
| # --- H2: Thread-safe provider singleton --- | ||
|
|
||
|
|
||
| def test_get_provider_thread_safe(): | ||
| """Concurrent calls to _get_provider return the same instance.""" | ||
| import threading | ||
|
|
||
| import src.llm_client as llm_mod | ||
|
|
||
| # Reset cached provider | ||
| llm_mod._cached_provider = None | ||
|
|
||
| results: list[int] = [] | ||
| barrier = threading.Barrier(3) | ||
|
|
||
| def get_provider_thread(): | ||
| barrier.wait() # Ensure all threads start simultaneously | ||
| provider = llm_mod._get_provider() | ||
| results.append(id(provider)) | ||
|
|
||
| with patch.object(llm_mod, "settings") as mock_settings: | ||
| mock_settings.llm_configured = True | ||
| mock_settings.llm_provider = "openai" | ||
| mock_settings.openai_api_key = "test-key" | ||
| mock_settings.openai_org_id = "" | ||
| mock_settings.openai_model = "gpt-4.1" | ||
| mock_settings.llm_timeout = 30.0 | ||
| mock_settings.llm_max_retries = 2 | ||
|
|
||
| threads = [threading.Thread(target=get_provider_thread) for _ in range(3)] | ||
| for t in threads: | ||
| t.start() | ||
| for t in threads: | ||
| t.join() | ||
|
|
||
| # All threads should get the same instance | ||
| assert len(set(results)) == 1, f"Expected 1 unique provider, got {len(set(results))}" | ||
|
|
||
| # Cleanup | ||
| llm_mod._cached_provider = None |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Verify the file path matches the patterns
echo "File: apps/intelligence/src/tests/test_llm_client.py"
echo "Pattern 1: apps/intelligence/**/*.py - matches: YES"
echo "Pattern 2: **/test_*.py - matches: YES"
echo ""
echo "Checking which guideline should apply when both patterns match..."Repository: lvlup-sw/authscript
Length of output: 273
🏁 Script executed:
# Find other test files in apps/intelligence
fd "test_.*\.py" apps/intelligence --type f
# Check if other test files in this directory have type hints
echo "=== Checking for type hints in other test files ==="
fd "test_.*\.py" apps/intelligence --type f | head -5 | xargs -I {} sh -c 'echo "File: {}"; head -30 {}'
# Check production code in apps/intelligence to see if type hints are strictly enforced
echo ""
echo "=== Checking production code for type hints pattern ==="
fd "\.py" apps/intelligence/src --type f ! -path "*/tests/*" | head -3 | xargs -I {} sh -c 'echo "File: {}"; head -40 {}'Repository: lvlup-sw/authscript
Length of output: 5165
🏁 Script executed:
# Search for test functions with explicit -> None return type
rg "def test_.*\(\)\s*->\s*None" apps/intelligence --type py
# Also check for async test functions with -> None
rg "async def test_.*\(\)\s*->\s*None" apps/intelligence --type py
# Count test functions without return type annotations
echo "=== Test functions in test_pdf_parser.py without return type ==="
rg "^\s*(async\s+)?def test_" apps/intelligence/src/tests/test_pdf_parser.py -A 1 --type py | head -20Repository: lvlup-sw/authscript
Length of output: 1501
Add return type annotations to match codebase conventions.
Test functions and helpers lack -> None return type annotations. Other test files in apps/intelligence/src/tests/ (such as test_analyze.py, test_evidence_extractor.py, and test_form_generator.py) include these annotations consistently. Add -> None to all test functions and the get_provider_thread() helper to align with the established pattern and satisfy the guideline requirement for complete type annotations.
🛠️ Example update (apply to all test functions and helpers)
-def test_get_provider_returns_singleton():
+def test_get_provider_returns_singleton() -> None:
@@
-def test_openai_provider_creates_client_once():
+def test_openai_provider_creates_client_once() -> None:
@@
-@pytest.mark.asyncio
-async def test_chat_completion_uses_provider_singleton():
+@pytest.mark.asyncio
+async def test_chat_completion_uses_provider_singleton() -> None:
@@
- def get_provider_thread():
+ def get_provider_thread() -> None:🤖 Prompt for AI Agents
In `@apps/intelligence/src/tests/test_llm_client.py` around lines 1 - 187, The
tests lack explicit return type annotations; update every test function
(test_get_provider_returns_singleton, test_openai_provider_creates_client_once,
test_chat_completion_uses_provider_singleton,
test_chat_completion_returns_none_on_timeout,
test_chat_completion_raises_on_rate_limit,
test_chat_completion_returns_none_on_api_error, test_get_provider_thread_safe)
and the helper get_provider_thread to include the return type -> None (e.g., def
test_xxx(...) -> None:) to match the codebase convention for complete type
annotations; no other behavior changes are needed.
Comment on lines
+1
to
+49
| """Tests for PDF parser thread pool and parallel execution.""" | ||
|
|
||
| import asyncio | ||
| import time | ||
|
|
||
| import pytest | ||
| from unittest.mock import patch, MagicMock | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_parse_pdf_does_not_block_event_loop(): | ||
| """Test that parse_pdf uses run_in_executor for sync operations.""" | ||
| from src.parsers.pdf_parser import parse_pdf | ||
|
|
||
| mock_extract = MagicMock(return_value="# Extracted Text\nSome content") | ||
|
|
||
| with patch("src.parsers.pdf_parser._extract_sync", mock_extract): | ||
| result = await parse_pdf(b"%PDF-1.4 fake content") | ||
|
|
||
| assert "Extracted Text" in result | ||
| mock_extract.assert_called_once() | ||
|
|
||
|
|
||
| @pytest.mark.asyncio | ||
| async def test_parse_pdf_multiple_docs_parallel(): | ||
| """Test that multiple PDFs can be parsed concurrently.""" | ||
| from src.parsers.pdf_parser import parse_pdf | ||
|
|
||
| call_times: list[float] = [] | ||
|
|
||
| def slow_extract(pdf_bytes): | ||
| call_times.append(time.monotonic()) | ||
| import time as t | ||
|
|
||
| t.sleep(0.05) # Simulate extraction time | ||
| return f"# Content from {len(pdf_bytes)} bytes" | ||
|
|
||
| with patch("src.parsers.pdf_parser._extract_sync", side_effect=slow_extract): | ||
| start = time.monotonic() | ||
| results = await asyncio.gather( | ||
| parse_pdf(b"pdf1"), | ||
| parse_pdf(b"pdf2"), | ||
| parse_pdf(b"pdf3"), | ||
| ) | ||
| duration = time.monotonic() - start | ||
|
|
||
| assert len(results) == 3 | ||
| # If parallel (thread pool): ~0.05s. If sequential: ~0.15s | ||
| assert duration < 0.12, f"Expected parallel execution (<0.12s), got {duration:.2f}s" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the test file
find . -name "test_pdf_parser.py" -type f 2>/dev/null | head -20Repository: lvlup-sw/authscript
Length of output: 111
🏁 Script executed:
# Check file size and read content if it exists
if [ -f "apps/intelligence/src/tests/test_pdf_parser.py" ]; then
echo "=== File found at apps/intelligence/src/tests/test_pdf_parser.py ==="
wc -l apps/intelligence/src/tests/test_pdf_parser.py
echo "=== Content ==="
head -60 apps/intelligence/src/tests/test_pdf_parser.py
else
echo "File not found at expected path"
fiRepository: lvlup-sw/authscript
Length of output: 1745
🏁 Script executed:
# Check if there are any other test_pdf_parser.py files in the repo
find . -path "*/test*" -name "*pdf*parser*" -type f 2>/dev/nullRepository: lvlup-sw/authscript
Length of output: 111
🏁 Script executed:
# Check other test files under apps/intelligence to see how they handle type hints
find ./apps/intelligence -name "test_*.py" -type f 2>/dev/null | head -5Repository: lvlup-sw/authscript
Length of output: 316
🏁 Script executed:
# Let's examine a couple of other test files to see the pattern
for file in $(find ./apps/intelligence -name "test_*.py" -type f 2>/dev/null | head -3); do
echo "=== File: $file ==="
head -30 "$file" | grep -A 2 "^async def\|^def " || echo "No function definitions in first 30 lines"
doneRepository: lvlup-sw/authscript
Length of output: 804
🏁 Script executed:
# Let's look at actual test functions in other test files to see if they have return type annotations
for file in $(find ./apps/intelligence -name "test_*.py" -type f 2>/dev/null | head -3); do
echo "=== File: $file ==="
grep -n "^async def test_\|^def test_" "$file" | head -5
doneRepository: lvlup-sw/authscript
Length of output: 1252
🏁 Script executed:
# Get more context around test functions in one of the other test files
head -60 ./apps/intelligence/src/tests/test_analyze.py | tail -40Repository: lvlup-sw/authscript
Length of output: 1432
Add return type annotations for the test functions and helper.
The codebase pattern in apps/intelligence/ requires complete type annotations on all functions. Both async test functions need -> None, and the slow_extract helper needs parameter and return type annotations.
Example update
-@pytest.mark.asyncio
-async def test_parse_pdf_does_not_block_event_loop():
+@pytest.mark.asyncio
+async def test_parse_pdf_does_not_block_event_loop() -> None:
@@
-@pytest.mark.asyncio
-async def test_parse_pdf_multiple_docs_parallel():
+@pytest.mark.asyncio
+async def test_parse_pdf_multiple_docs_parallel() -> None:
@@
- def slow_extract(pdf_bytes):
+ def slow_extract(pdf_bytes: bytes) -> str:🤖 Prompt for AI Agents
In `@apps/intelligence/src/tests/test_pdf_parser.py` around lines 1 - 49, Add
missing type annotations: annotate both async test functions
test_parse_pdf_does_not_block_event_loop and
test_parse_pdf_multiple_docs_parallel with return type -> None, and add type
hints to the helper slow_extract as (pdf_bytes: bytes) -> str; update its
signature and any local variables' annotations if necessary so the functions
follow the codebase pattern of complete function type annotations.
… handling, config validation - Add specific paRequest(id) query invalidation to approve/deny mutation hooks to prevent stale cache when viewing individual requests - Remove redundant _safe_parse wrapper in analyze.py since parse_pdf already handles errors internally - Add Pydantic Field constraints (ge/gt) to LLM performance settings to reject invalid environment values at startup Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
rsalus
changed the base branch from
feature/clafonso/dashboardBackendIntegration
to
main
Contributor
Author
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
apps/dashboard/src/api/customFetch.ts (1)
69-107:⚠️ Potential issue | 🟡 MinorUpdate the auth comment to reflect authscript_session priority.
The block says “SMART on FHIR session storage,” but the code now delegates togetAuthToken()which checksauthscript_sessionfirst. This is a minor doc accuracy mismatch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/api/customFetch.ts` around lines 69 - 107, Update the comment above customFetch to accurately state that authorization is obtained via getAuthToken (which prefers the authscript_session value) rather than generically saying “SMART on FHIR session storage”; mention getAuthToken by name and that it checks authscript_session first so readers know the priority and source of the token used for setting the Authorization header in customFetch.apps/intelligence/src/llm_client.py (1)
198-216:⚠️ Potential issue | 🟠 MajorRemove the generic
except Exceptioncatch-all fromchat_completion.The catch swallows unexpected failures and violates the coding guideline requiring specific exceptions over bare except. The function already handles all documented OpenAI client exceptions (APITimeoutError, RateLimitError, APIError). Any other exception should propagate rather than being silently logged and suppressed.
Proposed fix
- except Exception as e: - logger.error("Unexpected LLM error: %s", e) - return None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/intelligence/src/llm_client.py` around lines 198 - 216, The generic "except Exception" block in chat_completion should be removed so unexpected errors are not swallowed; keep the existing specific handlers for APITimeoutError, RateLimitError and APIError (referencing CompletionRequest, provider.complete, logger) and allow any other exceptions to propagate to the caller rather than catching and logging them silently. Ensure the function still returns None for handled cases (timeout/APIError) and re-raises RateLimitError as currently implemented.
🧹 Nitpick comments (11)
apps/gateway/Gateway.API/appsettings.Development.json (1)
8-10: Keep OAuth client configuration environment-scoped.
Athena:ClientIdis environment-specific; consider moving it to user-secrets/env vars to prevent accidental reuse across environments and keep appsettings as a non-sensitive template.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/gateway/Gateway.API/appsettings.Development.json` around lines 8 - 10, The Athena:ClientId entry in the Athena configuration block is environment-specific and should be removed from appsettings.Development.json and moved to an environment-secret mechanism (user-secrets or environment variable); update code that reads the client id (where you reference "Athena:ClientId" via IConfiguration or options binding) to fall back to IConfiguration["Athena:ClientId"] (or an injected IOptions<AthenaOptions>.Value.ClientId) so it reads from env/user-secrets at runtime, and leave the appsettings Athena section as a non-sensitive template (e.g., keep FhirBaseUrl but not ClientId).apps/gateway/Gateway.API.Tests/Services/Polling/AthenaPollingServiceTests.cs (1)
231-263: Consider applying the sameTaskCompletionSourcepattern here for consistency.This test still uses
Task.Delay(100)(line 247), while the updated test above uses the more deterministicTaskCompletionSourceapproach. Aligning the patterns across similar tests would improve consistency and reduce potential flakiness.♻️ Optional refactor for consistency
[Test] public async Task ExecuteAsync_WithRegisteredPatients_CallsGetActiveAsync() { // Arrange + var getActiveCalled = new TaskCompletionSource(TaskCreationOptions.RunContinuationsAsynchronously); var patients = new List<RegisteredPatient> { new() { PatientId = "p1", EncounterId = "e1", PracticeId = "pr1", WorkItemId = "w1", RegisteredAt = DateTimeOffset.UtcNow }, new() { PatientId = "p2", EncounterId = "e2", PracticeId = "pr2", WorkItemId = "w2", RegisteredAt = DateTimeOffset.UtcNow }, }; _patientRegistry.GetActiveAsync(Arg.Any<CancellationToken>()) - .Returns(patients); + .Returns(patients) + .AndDoes(_ => getActiveCalled.TrySetResult()); using var cts = new CancellationTokenSource(); // Act var task = _sut.StartAsync(cts.Token); - await Task.Delay(100); + await Task.WhenAny(getActiveCalled.Task, Task.Delay(TimeSpan.FromSeconds(10))); cts.Cancel();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/gateway/Gateway.API.Tests/Services/Polling/AthenaPollingServiceTests.cs` around lines 231 - 263, The test ExecuteAsync_WithRegisteredPatients_CallsGetActiveAsync is using Task.Delay(100) which is flaky; replace that timing-based wait with a TaskCompletionSource that completes when _patientRegistry.GetActiveAsync is invoked. Update the _patientRegistry.GetActiveAsync(Arg.Any<CancellationToken>()) setup to call tcs.SetResult(true) (or similar) inside its Returns callback and have the test await tcs.Task instead of Task.Delay before cancelling the CancellationTokenSource; keep the rest of the test assertions (StartAsync/StopAsync and Received() call) unchanged.apps/dashboard/src/lib/patients.ts (1)
22-22: Consider marking the test data asreadonly.Per coding guidelines,
readonlyshould be the default for types. Since this array is test fixture data that shouldn't be mutated at runtime, marking it immutable improves safety.♻️ Proposed fix
-export const ATHENA_TEST_PATIENTS: Patient[] = [ +export const ATHENA_TEST_PATIENTS: readonly Patient[] = [As per coding guidelines: "
apps/dashboard/**/*.ts: Types: ... readonly default".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/lib/patients.ts` at line 22, The ATHENA_TEST_PATIENTS test fixture is mutable but should be immutable; change its type annotation from Patient[] to a readonly type (e.g., export const ATHENA_TEST_PATIENTS: readonly Patient[] = […] or export const ATHENA_TEST_PATIENTS: ReadonlyArray<Patient> = […] ) so the array cannot be mutated at runtime and matches the readonly-by-default guideline.apps/gateway/Gateway.API/Services/DocumentUploader.cs (1)
35-35: Redundant fully qualified type names.
Gateway.API.Contractsis already imported (line 3), soGateway.API.Contracts.Result<string>can be simplified toResult<string>. If there's a naming collision with anotherResulttype, consider using a type alias for clarity.🧹 Suggested simplification
- public async Task<Gateway.API.Contracts.Result<string>> UploadDocumentAsync( + public async Task<Result<string>> UploadDocumentAsync(- return Gateway.API.Contracts.Result<string>.Failure(result.Error!); + return Result<string>.Failure(result.Error!);- return Gateway.API.Contracts.Result<string>.Success(documentId); + return Result<string>.Success(documentId);Also applies to: 55-55, 72-72
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/gateway/Gateway.API/Services/DocumentUploader.cs` at line 35, The method signatures (e.g., UploadDocumentAsync) and other places using the fully qualified type Gateway.API.Contracts.Result<string> should be simplified to Result<string> since Gateway.API.Contracts is already imported; update all occurrences (including the other Result<string> uses in the same class) to the shorter form, or if there is a naming collision with another Result type, add a using alias (e.g., using ContractsResult = Gateway.API.Contracts.Result<string>) and use that alias in the method signatures and return statements to keep intent clear.apps/gateway/Gateway.API/Services/Fhir/FhirHttpClient.cs (1)
33-33: Remove redundant fully qualified type names.
Gateway.API.Contractsis imported on line 4, so the fully qualified references toResult<T>are unnecessary. Using the imported namespace directly reduces visual noise across all four async methods.Applies to lines: 33, 53, 68, 88, 103, 130, 145, 164
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/gateway/Gateway.API/Services/Fhir/FhirHttpClient.cs` at line 33, The method signatures in FhirHttpClient use redundant fully qualified types like Gateway.API.Contracts.Result<T>; update each async method signature (e.g., ReadAsync and the other async methods in the class) to use the imported namespace type name (Result<T>) instead of Gateway.API.Contracts.Result<T> for JsonElement, string, etc., and do the same for their return types and any related local variable/type references so the code relies on the using import and removes the visual noise.apps/dashboard/src/components/SubmissionProgressOverlay.tsx (1)
9-10: Consolidate lucide-react imports.Minor nit: These two imports can be merged into a single statement.
♻️ Suggested consolidation
-import { Search, Check, CheckCircle2 } from 'lucide-react'; -import { Sparkles } from 'lucide-react'; +import { Search, Check, CheckCircle2, Sparkles } from 'lucide-react';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/components/SubmissionProgressOverlay.tsx` around lines 9 - 10, The imports from 'lucide-react' are split; consolidate them into a single import statement that imports Search, Check, CheckCircle2 and Sparkles together (update the import that currently declares Search, Check, CheckCircle2 and the separate Sparkles import in SubmissionProgressOverlay.tsx) so all lucide-react icons are imported in one line.apps/gateway/Gateway.API.Tests/Services/LiveDataServiceTests.cs (1)
11-22: Seal the test class to match gateway conventions.
This keeps consistency with the “sealed by default” guideline.🛠️ Suggested change
-public class LiveDataServiceTests +public sealed class LiveDataServiceTestsAs per coding guidelines, “Types: Sealed by default, records for DTOs, nullable enabled”.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/gateway/Gateway.API.Tests/Services/LiveDataServiceTests.cs` around lines 11 - 22, The test class LiveDataServiceTests should be declared sealed to follow the "sealed by default" convention; update the class declaration for LiveDataServiceTests to include the sealed modifier (i.e., make class LiveDataServiceTests sealed) so the test type follows gateway conventions and matches other test classes.apps/dashboard/src/api/graphqlService.ts (1)
199-272: Default exported interfaces to readonly properties.
These shapes are reused across the app; marking fieldsreadonlyby default will better align with the project’s TypeScript standard.♻️ Example adjustment (apply broadly)
export interface PAStats { - ready: number; - submitted: number; - waitingForInsurance: number; - attention: number; - total: number; + readonly ready: number; + readonly submitted: number; + readonly waitingForInsurance: number; + readonly attention: number; + readonly total: number; }As per coding guidelines, “Types: Interfaces for extendable shapes, readonly default, no
any”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/api/graphqlService.ts` around lines 199 - 272, Make all interface properties readonly to follow project TS conventions: update Patient, Procedure, Medication, Criterion, PARequest, PAStats, and ActivityItem so every field is prefixed with readonly (including nested object properties like PARequest.patient) and convert arrays to readonly types (e.g., PARequest.criteria -> ReadonlyArray<Criterion>); preserve optional/union types (e.g., readyAt?: string | null) but mark them readonly as well so the shapes are immutable by default.apps/dashboard/src/routes/analysis.$transactionId.tsx (1)
41-250: Consider extracting helper components and defining prop interfaces.
ProgressRing/EditableField/CriteriaItem/CriteriaReasonDialog are all inline with anonymous prop types; moving them to their own files and addingPropsinterfaces will better match the project standards.
As per coding guidelines, “Components: One component per file, use functional components with hooks” and “Props: Define interface for props, use destructuring”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/routes/analysis`.$transactionId.tsx around lines 41 - 250, The four inline components (ProgressRing, EditableField, CriteriaItem, CriteriaReasonDialog) should be moved to separate files and given explicit Props interfaces: create ProgressRing.tsx, EditableField.tsx, CriteriaItem.tsx, CriteriaReasonDialog.tsx; define and export an interface (e.g., ProgressRingProps, EditableFieldProps, CriteriaItemProps, CriteriaReasonDialogProps) describing each prop shape (including types like number, boolean, string, optional fields and callbacks), convert the anonymous inline props to those interfaces and use destructured parameters in the functional components, export them as default (or named) and import them back into the parent route file; ensure any helper utilities used (displayConfidence, cn, icon components, createPortal) are imported where needed and update imports/exports accordingly to satisfy the “one component per file” and prop-interface guidelines.apps/dashboard/src/routes/index.tsx (1)
33-120: Extract helper components and define props interfaces.
StatCard/WorkflowPipeline (and other helpers in this file) are inline with anonymous prop types; splitting them into separate files with explicitPropsinterfaces will align with the standards.
As per coding guidelines, “Components: One component per file, use functional components with hooks” and “Props: Define interface for props, use destructuring”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/dashboard/src/routes/index.tsx` around lines 33 - 120, Move the inline components into their own files and add explicit props interfaces: create StatCard.tsx exporting the StatCard component and define a StatCardProps interface for { label: string; value: string | number; change?: string; positive?: boolean; icon: React.ElementType }, then import and use it where needed; similarly create WorkflowPipeline.tsx with a WorkflowPipelineProps interface if it needs props (or export as a standalone component) and extract the internal steps array or accept it via props; ensure each file uses a named functional component (not anonymous) and default/consistent exports and update any imports in the parent file accordingly (refer to StatCard and WorkflowPipeline in the diff).apps/intelligence/src/api/analyze.py (1)
100-103: Consider bounding PDF reads/parses to avoid memory spikes.
Reading all document bytes upfront keeps every PDF resident in memory; with many/large uploads this can spike RAM. Consider batching with a semaphore or streaming each file into parse tasks so bytes can be released sooner.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/intelligence/src/api/analyze.py` around lines 100 - 103, The code currently reads all documents into pdf_bytes_list then calls document_texts = list(await asyncio.gather(*[parse_pdf(b) for b in pdf_bytes_list])), which keeps every PDF in memory; change to bounded concurrency so bytes are released sooner: iterate documents and create tasks that call parse_pdf on each stream/read chunk but gate concurrent parse_pdf calls with an asyncio.Semaphore (or process in small batches) so you only hold a few pdf_bytes at once; reference pdf_bytes_list, parse_pdf, documents, and document_texts and ensure each read/parse task releases its bytes when done so memory usage is bounded.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/dashboard/package.json`:
- Around line 33-36: The package.json entry for the dependency "graphql-request"
is pinned to "^7.0.0" and needs to be updated to "^7.4.0"; open
apps/dashboard/package.json, locate the "graphql-request" dependency line and
bump the version string to 7.4.0 (keeping the existing caret if desired), then
run yarn/npm install and update lockfile to ensure the new version is recorded.
In `@apps/dashboard/src/lib/patients.ts`:
- Around line 7-20: The exported ATHENA_TEST_PATIENTS array should be typed as
readonly to follow coding guidelines: update its annotation to readonly
Patient[] (or use as const) so the test data is immutable and TypeScript
enforces it; also to avoid future type collisions, rename the other Patient
interface in graphqlService.ts to AthenaTestPatient (update its usages/imports
accordingly) so the distinct shapes are clear and not accidentally mixed up.
In `@apps/dashboard/src/lib/pdfGenerator.ts`:
- Around line 365-418: The current generatePAPdfBlob function builds CSS by
using doc.head.innerHTML which can include non-CSS nodes like <title>, causing
invalid CSS in the injected <style>; change the logic that computes styles
(currently using doc.head.innerHTML.replace(...)) to instead collect only CSS
text from the document's <style> elements (e.g. iterate
doc.querySelectorAll('head style'), join their .textContent, then perform the
body selector replacement like .replace(/\bbody\b/g, `#${PDF_CONTAINER_ID}`)).
Keep the rest of the DOM insertion (container.id = PDF_CONTAINER_ID,
container.innerHTML) the same so only valid CSS is injected and other head nodes
(title/meta) aren’t wrapped into the <style>.
In `@apps/dashboard/src/routes/analysis`.$transactionId.tsx:
- Around line 926-940: The PDF modal is receiving the original request variable
(prop request) so it can show outdated data when reanalysis overrides
criteria/confidence; update the code so PdfViewerModal receives the effective
request used by the UI (e.g., a computed value like effectiveRequest,
requestWithOverrides, or the same variable you render elsewhere after applying
reanalysis overrides) instead of the raw request; locate the PdfViewerModal JSX
and replace the request prop with that effective request value (ensuring that
the effectiveRequest is computed/derived earlier in the component using the same
override logic as the UI).
In `@apps/dashboard/src/routes/index.tsx`:
- Around line 143-148: The formatDuration function can produce "0m 60s" because
it uses Math.round for seconds; change the logic in formatDuration to compute
seconds with Math.floor (e.g., secs = Math.floor((ms % 60000) / 1000)) or
otherwise guard against secs === 60 by incrementing mins and setting secs = 0,
so the output never shows "60s" (update the variables ms, mins, secs in function
formatDuration accordingly).
In `@apps/gateway/Gateway.API/appsettings.Development.json`:
- Around line 2-4: Remove the hardcoded credentials from the
"ConnectionStrings": { "authscript": ... } entry and replace the value with a
non-secret placeholder (e.g.,
"Host=localhost;Port=5433;Database=authscript;Username={DB_USER};Password={DB_PASS}")
in appsettings.Development.json, then load the real connection string from a
secure source at runtime (environment variable or user-secrets) by wiring the
IConfiguration to read the environment key used (e.g.,
Configuration["ConnectionStrings:authscript"] or an env var like
AUTH_SCRIPT_CONNECTIONSTRING) and ensure any code that references
ConnectionStrings:authscript uses that configuration value instead of the
committed secret.
In `@apps/gateway/Gateway.API/Gateway.API.csproj`:
- Around line 44-46: Update the HotChocolate.AspNetCore package reference in
Gateway.API.csproj from Version="15.1.0" to Version="15.1.12" to pick up the
latest 15.x patch fixes and improved .NET 10 compatibility; locate the
<PackageReference Include="HotChocolate.AspNetCore" Version="15.1.0" /> entry
and change the Version attribute to "15.1.12", then restore/update NuGet
packages and run the project/tests to verify no regressions.
In `@apps/gateway/Gateway.API/GraphQL/Inputs/UpdatePARequestInput.cs`:
- Around line 1-13: The file currently declares two public types
(UpdatePARequestInput and CriterionInput), violating the
one-public-type-per-file rule; fix by either moving CriterionInput into its own
file as a public sealed record named CriterionInput(bool? Met, string Label,
string? Reason = null) or change its accessibility to non-public (e.g., internal
sealed record CriterionInput(...)) so only UpdatePARequestInput remains public;
update any references/imports accordingly to compile.
In `@apps/intelligence/src/reasoning/evidence_extractor.py`:
- Around line 137-181: The loop in extract_evidence treats any BaseException
returned from asyncio.gather as a recoverable error and converts cancellations
into "UNCLEAR" results; change the handling to let asyncio.CancelledError
propagate by checking for asyncio.CancelledError first and re-raising it (or by
only treating instances of Exception as recoverable), i.e., in the results
processing inside extract_evidence (which handles values from _bounded_evaluate)
detect asyncio.CancelledError and raise it, otherwise log and convert other
exceptions into an EvidenceItem as before.
---
Outside diff comments:
In `@apps/dashboard/src/api/customFetch.ts`:
- Around line 69-107: Update the comment above customFetch to accurately state
that authorization is obtained via getAuthToken (which prefers the
authscript_session value) rather than generically saying “SMART on FHIR session
storage”; mention getAuthToken by name and that it checks authscript_session
first so readers know the priority and source of the token used for setting the
Authorization header in customFetch.
In `@apps/intelligence/src/llm_client.py`:
- Around line 198-216: The generic "except Exception" block in chat_completion
should be removed so unexpected errors are not swallowed; keep the existing
specific handlers for APITimeoutError, RateLimitError and APIError (referencing
CompletionRequest, provider.complete, logger) and allow any other exceptions to
propagate to the caller rather than catching and logging them silently. Ensure
the function still returns None for handled cases (timeout/APIError) and
re-raises RateLimitError as currently implemented.
---
Duplicate comments:
In `@apps/gateway/Gateway.API/GraphQL/Mutations/Mutation.cs`:
- Around line 78-84: The ApprovePARequest and DenyPARequest methods use
expression-bodied (arrow) syntax which violates the "no arrow code" guideline;
change both to regular block-bodied methods matching the surrounding style:
implement ApprovePARequest(string id, [Service] IDataService dataService) {
return dataService.ApprovePA(id); } and DenyPARequest(string id, string reason,
[Service] IDataService dataService) { return dataService.DenyPA(id, reason); }
so callers (and the file) use consistent method bodies while keeping calls to
IDataService.ApprovePA and IDataService.DenyPA unchanged.
In `@apps/gateway/Gateway.API/GraphQL/Queries/Query.cs`:
- Around line 17-51: Convert all GraphQL resolver methods (GetProcedures,
GetMedications, GetProviders, GetPayers, GetDiagnoses, GetPARequests,
GetPARequest, GetPAStats, GetActivity) from expression-bodied to block-bodied
methods and add guard clauses at the top: validate that the injected
IDataService parameter is not null (throw
ArgumentNullException(nameof(dataService)) or return an appropriate default) and
validate the id parameter in GetPARequest (throw ArgumentNullException or return
null if id is null/empty); then call the corresponding IDataService member and
return its result. Ensure each method uses an explicit block body (no =>) and
performs the early-return/guard-first pattern required by the coding guidelines.
In `@apps/gateway/Gateway.API/Services/LiveDataService.cs`:
- Around line 18-84: The members in LiveDataService (e.g., Procedures,
Medications, Providers, Payers, Diagnoses, GetPARequests, GetPARequest,
GetPAStats, GetActivity, CreatePARequest, UpdatePARequest,
ProcessPARequestAsync, SubmitPARequest, AddReviewTime, DeletePARequest,
ApprovePA, DenyPA) use expression-bodied syntax (=>) which violates the "no
arrow code" guideline—replace each expression-bodied property or method with a
block-bodied implementation that simply throws the same NotImplementedException
(preserving the existing message text), e.g. change the expression-bodied
property to a get { throw new
NotImplementedException("LiveDataService.Procedures not yet implemented."); }
and change methods to { throw new
NotImplementedException("LiveDataService.MethodName not yet implemented."); } so
all members use block bodies instead of arrow expressions.
In `@apps/intelligence/src/tests/test_evidence_extractor.py`:
- Around line 78-223: Tests and helper coroutines are missing explicit return
type annotations; add them to satisfy the project's type-hint rule. Update each
async test function (e.g., test_evaluate_criterion_returns_met_evidence_item,
test_evaluate_criterion_parses_not_met,
test_evaluate_criterion_handles_none_response,
test_extract_evidence_calls_criteria_concurrently,
test_extract_evidence_respects_semaphore_limit) to declare a return type of ->
None, and annotate helper async coroutines (slow_llm, counting_llm) with their
concrete return type (e.g., -> str); if you use Coroutine types instead, import
typing symbols (Any, Coroutine) and annotate accordingly. Ensure imports are
added if necessary and keep signatures consistent with their return values so
the module passes the type-annotation rule.
In `@apps/intelligence/src/tests/test_llm_client.py`:
- Around line 7-187: Several test functions and the helper get_provider_thread
are missing return type annotations; update each function definition
(test_get_provider_returns_singleton, test_openai_provider_creates_client_once,
test_chat_completion_uses_provider_singleton,
test_chat_completion_returns_none_on_timeout,
test_chat_completion_raises_on_rate_limit,
test_chat_completion_returns_none_on_api_error, test_get_provider_thread_safe,
and the inner get_provider_thread) to include explicit return types (use -> None
for regular and async functions) so every function has a complete type
annotation per the project guideline.
In `@apps/intelligence/src/tests/test_pdf_parser.py`:
- Around line 10-49: Add explicit type hints: annotate both async test functions
test_parse_pdf_does_not_block_event_loop and
test_parse_pdf_multiple_docs_parallel with a return type of -> None, and add
type annotations to the helper used in the second test (slow_extract) including
its parameter and return type (e.g., bytes -> str) so the functions comply with
the project-wide typing rule; update any local MagicMock definitions if needed
to include types (e.g., mock_extract: MagicMock) and import typing symbols if
not already present.
---
Nitpick comments:
In `@apps/dashboard/src/api/graphqlService.ts`:
- Around line 199-272: Make all interface properties readonly to follow project
TS conventions: update Patient, Procedure, Medication, Criterion, PARequest,
PAStats, and ActivityItem so every field is prefixed with readonly (including
nested object properties like PARequest.patient) and convert arrays to readonly
types (e.g., PARequest.criteria -> ReadonlyArray<Criterion>); preserve
optional/union types (e.g., readyAt?: string | null) but mark them readonly as
well so the shapes are immutable by default.
In `@apps/dashboard/src/components/SubmissionProgressOverlay.tsx`:
- Around line 9-10: The imports from 'lucide-react' are split; consolidate them
into a single import statement that imports Search, Check, CheckCircle2 and
Sparkles together (update the import that currently declares Search, Check,
CheckCircle2 and the separate Sparkles import in SubmissionProgressOverlay.tsx)
so all lucide-react icons are imported in one line.
In `@apps/dashboard/src/lib/patients.ts`:
- Line 22: The ATHENA_TEST_PATIENTS test fixture is mutable but should be
immutable; change its type annotation from Patient[] to a readonly type (e.g.,
export const ATHENA_TEST_PATIENTS: readonly Patient[] = […] or export const
ATHENA_TEST_PATIENTS: ReadonlyArray<Patient> = […] ) so the array cannot be
mutated at runtime and matches the readonly-by-default guideline.
In `@apps/dashboard/src/routes/analysis`.$transactionId.tsx:
- Around line 41-250: The four inline components (ProgressRing, EditableField,
CriteriaItem, CriteriaReasonDialog) should be moved to separate files and given
explicit Props interfaces: create ProgressRing.tsx, EditableField.tsx,
CriteriaItem.tsx, CriteriaReasonDialog.tsx; define and export an interface
(e.g., ProgressRingProps, EditableFieldProps, CriteriaItemProps,
CriteriaReasonDialogProps) describing each prop shape (including types like
number, boolean, string, optional fields and callbacks), convert the anonymous
inline props to those interfaces and use destructured parameters in the
functional components, export them as default (or named) and import them back
into the parent route file; ensure any helper utilities used (displayConfidence,
cn, icon components, createPortal) are imported where needed and update
imports/exports accordingly to satisfy the “one component per file” and
prop-interface guidelines.
In `@apps/dashboard/src/routes/index.tsx`:
- Around line 33-120: Move the inline components into their own files and add
explicit props interfaces: create StatCard.tsx exporting the StatCard component
and define a StatCardProps interface for { label: string; value: string |
number; change?: string; positive?: boolean; icon: React.ElementType }, then
import and use it where needed; similarly create WorkflowPipeline.tsx with a
WorkflowPipelineProps interface if it needs props (or export as a standalone
component) and extract the internal steps array or accept it via props; ensure
each file uses a named functional component (not anonymous) and
default/consistent exports and update any imports in the parent file accordingly
(refer to StatCard and WorkflowPipeline in the diff).
In `@apps/gateway/Gateway.API.Tests/Services/LiveDataServiceTests.cs`:
- Around line 11-22: The test class LiveDataServiceTests should be declared
sealed to follow the "sealed by default" convention; update the class
declaration for LiveDataServiceTests to include the sealed modifier (i.e., make
class LiveDataServiceTests sealed) so the test type follows gateway conventions
and matches other test classes.
In
`@apps/gateway/Gateway.API.Tests/Services/Polling/AthenaPollingServiceTests.cs`:
- Around line 231-263: The test
ExecuteAsync_WithRegisteredPatients_CallsGetActiveAsync is using Task.Delay(100)
which is flaky; replace that timing-based wait with a TaskCompletionSource that
completes when _patientRegistry.GetActiveAsync is invoked. Update the
_patientRegistry.GetActiveAsync(Arg.Any<CancellationToken>()) setup to call
tcs.SetResult(true) (or similar) inside its Returns callback and have the test
await tcs.Task instead of Task.Delay before cancelling the
CancellationTokenSource; keep the rest of the test assertions
(StartAsync/StopAsync and Received() call) unchanged.
In `@apps/gateway/Gateway.API/appsettings.Development.json`:
- Around line 8-10: The Athena:ClientId entry in the Athena configuration block
is environment-specific and should be removed from appsettings.Development.json
and moved to an environment-secret mechanism (user-secrets or environment
variable); update code that reads the client id (where you reference
"Athena:ClientId" via IConfiguration or options binding) to fall back to
IConfiguration["Athena:ClientId"] (or an injected
IOptions<AthenaOptions>.Value.ClientId) so it reads from env/user-secrets at
runtime, and leave the appsettings Athena section as a non-sensitive template
(e.g., keep FhirBaseUrl but not ClientId).
In `@apps/gateway/Gateway.API/Services/DocumentUploader.cs`:
- Line 35: The method signatures (e.g., UploadDocumentAsync) and other places
using the fully qualified type Gateway.API.Contracts.Result<string> should be
simplified to Result<string> since Gateway.API.Contracts is already imported;
update all occurrences (including the other Result<string> uses in the same
class) to the shorter form, or if there is a naming collision with another
Result type, add a using alias (e.g., using ContractsResult =
Gateway.API.Contracts.Result<string>) and use that alias in the method
signatures and return statements to keep intent clear.
In `@apps/gateway/Gateway.API/Services/Fhir/FhirHttpClient.cs`:
- Line 33: The method signatures in FhirHttpClient use redundant fully qualified
types like Gateway.API.Contracts.Result<T>; update each async method signature
(e.g., ReadAsync and the other async methods in the class) to use the imported
namespace type name (Result<T>) instead of Gateway.API.Contracts.Result<T> for
JsonElement, string, etc., and do the same for their return types and any
related local variable/type references so the code relies on the using import
and removes the visual noise.
In `@apps/intelligence/src/api/analyze.py`:
- Around line 100-103: The code currently reads all documents into
pdf_bytes_list then calls document_texts = list(await
asyncio.gather(*[parse_pdf(b) for b in pdf_bytes_list])), which keeps every PDF
in memory; change to bounded concurrency so bytes are released sooner: iterate
documents and create tasks that call parse_pdf on each stream/read chunk but
gate concurrent parse_pdf calls with an asyncio.Semaphore (or process in small
batches) so you only hold a few pdf_bytes at once; reference pdf_bytes_list,
parse_pdf, documents, and document_texts and ensure each read/parse task
releases its bytes when done so memory usage is bounded.
Comment on lines
+33
to
+36
| "graphql": "^16.9.0", | ||
| "graphql-request": "^7.0.0", | ||
| "html2canvas": "^1.4.1", | ||
| "jspdf": "^4.1.0", |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
graphql 16.9.0 security advisories vulnerabilities
💡 Result:
For the npm package graphql (GraphQL.js) version 16.9.0, there are no known direct vulnerabilities/security advisories specifically affecting 16.9.0 in common public trackers. Snyk reports “No direct vulnerabilities” for graphql@16.9.0. [1]
The main widely-referenced security issue for GraphQL.js in the 16.x line is:
- CVE-2023-26144 / SNYK-JS-GRAPHQL-5905181 (DoS via query validation) — affects
graphql>=16.3.0 and <16.8.1; fixed in 16.8.1, so 16.9.0 is not affected. [2][3]
Also, GitHub’s security page for graphql/graphql-js indicates there are no published GitHub Security Advisories in that repository. [4]
Bottom line: if you’re on graphql@16.9.0, you’re already past the known affected range for CVE-2023-26144, and there are no other commonly listed advisories targeting 16.9.0 in these sources. [1][2][3]
Sources: [1] Snyk package page for graphql@16.9.0 • [2] NVD CVE-2023-26144 • [3] Snyk advisory SNYK-JS-GRAPHQL-5905181 • [4] GitHub graphql-js security overview.
🌐 Web query:
graphql-request 7.0.0 latest version security
💡 Result:
- Is
graphql-request@7.0.0the latest? No — the latest published version is 7.4.0. [1][2][3] - Known security issues in 7.x? Snyk reports no known direct vulnerabilities for
graphql-request(latest/non-vulnerable: 7.4.0). [1] - What to do: Upgrade to 7.4.0 (and still run
npm audit/your SCA scanner to catch transitive dependency issues). [1]
Sources: Snyk [1], Socket versions [2], jsDocs package metadata [3].
🌐 Web query:
html2canvas 1.4.1 security advisories
💡 Result:
- GitHub Security Advisories (niklasvh/html2canvas): No published security advisories are listed for the upstream repository. [1]
- Snyk (npm
html2canvas): Reports no known direct vulnerabilities forhtml2canvas, including v1.4.1 (listed as latest/non-vulnerable). [2], [3] - ReversingLabs (npm
html2canvas): Reports no known vulnerabilities detected for the package. [4], [5] - Meterian: Flags
html2canvas@1.4.1as “affected by 1 vulnerability” (severity shown as “Suggest”), but the public page does not provide details/IDs. [6]
Note: There are reported malicious-package advisories for similarly named packages like feedback-html2canvas, which is not the same as html2canvas. [7], [8]
Sources: [1]-[8] correspond to the citations above.
🌐 Web query:
jspdf 4.1.0 latest version security
💡 Result:
-
Latest jsPDF version: v4.1.0 (tagged Feb 2, 2026) and marked “Latest” on the project’s GitHub releases page. [1]
-
Security status of 4.1.0: the v4.1.0 release explicitly states it “fixes several security issues”, including:
- PDF Injection in AcroForm (arbitrary JavaScript execution in generated PDFs)
- Stored XMP metadata injection
- Shared-state race condition in
addJS - DoS via unvalidated BMP dimensions (ties to CVE-2026-24133, fixed in 4.1.0) [1][2]
-
Important prior issue (fixed earlier, but relevant if you’re on older versions):
- Local File Inclusion / Path Traversal in Node.js builds (GHSA-f8cm-6447-x5h2 / CVE-2025-68428) is fixed in v4.0.0 by restricting filesystem access by default. [3][4]
Recommendation: Upgrade to jsPDF 4.1.0 (or newer, if available later) to pick up the latest security fixes. If you use jsPDF on Node.js, follow the project guidance to enforce filesystem restrictions with Node’s permission flags. [4]
Sources: [1] [2] [3] [4]
Upgrade graphql-request to 7.4.0 before release.
The pinned versions are largely secure: graphql@16.9.0 (no vulnerabilities; past CVE-2023-26144 range), html2canvas@1.4.1 (no direct issues), and jsPDF@4.1.0 (latest, with recent security fixes for PDF injection and DoS). However, graphql-request@7.0.0 is outdated—upgrade to 7.4.0 (patch-level compatible) to stay current.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/dashboard/package.json` around lines 33 - 36, The package.json entry for
the dependency "graphql-request" is pinned to "^7.0.0" and needs to be updated
to "^7.4.0"; open apps/dashboard/package.json, locate the "graphql-request"
dependency line and bump the version string to 7.4.0 (keeping the existing caret
if desired), then run yarn/npm install and update lockfile to ensure the new
version is recorded.
Comment on lines
+7
to
+20
| export interface Patient { | ||
| id: string; | ||
| /** Athena Patient ID (numeric) */ | ||
| patientId: string; | ||
| /** FHIR R4 Logical ID */ | ||
| fhirId: string; | ||
| name: string; | ||
| mrn: string; | ||
| dob: string; | ||
| memberId: string; | ||
| payer: string; | ||
| address: string; | ||
| phone: string; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find all Patient interface definitions
rg -n --type=ts "interface Patient|type Patient" apps/dashboard/src/Repository: lvlup-sw/authscript
Length of output: 392
🏁 Script executed:
# Check the graphqlService.ts file specifically for the Patient interface
cat -n apps/dashboard/src/api/graphqlService.ts | sed -n '190,220p'Repository: lvlup-sw/authscript
Length of output: 859
🏁 Script executed:
# Check the patients.ts file to see the full context
cat -n apps/dashboard/src/lib/patients.ts | head -30Repository: lvlup-sw/authscript
Length of output: 943
🏁 Script executed:
# Search for imports of Patient across the codebase
rg -n --type=ts "import.*Patient.*from" apps/dashboard/src/Repository: lvlup-sw/authscript
Length of output: 179
🏁 Script executed:
# See how Patient type is used in assignments and type annotations
rg -n --type=ts ": Patient|Patient\s*\[|as Patient" apps/dashboard/src/ | head -50Repository: lvlup-sw/authscript
Length of output: 514
Apply readonly modifier to exported test patient array per coding guidelines.
The ATHENA_TEST_PATIENTS constant should be readonly by default. Change the type annotation to readonly Patient[] or use as const to align with TypeScript standards.
Additionally, there is a second Patient interface defined in apps/dashboard/src/api/graphqlService.ts with a different shape (missing patientId and fhirId). While the current imports are well-separated and there's no active type confusion, consider renaming this interface to AthenaTestPatient to improve clarity and prevent accidental misuse in the future, especially as the codebase grows.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/dashboard/src/lib/patients.ts` around lines 7 - 20, The exported
ATHENA_TEST_PATIENTS array should be typed as readonly to follow coding
guidelines: update its annotation to readonly Patient[] (or use as const) so the
test data is immutable and TypeScript enforces it; also to avoid future type
collisions, rename the other Patient interface in graphqlService.ts to
AthenaTestPatient (update its usages/imports accordingly) so the distinct shapes
are clear and not accidentally mixed up.
Comment on lines
+365
to
+418
| export async function generatePAPdfBlob(request: PARequest): Promise<Blob> { | ||
| const html = generatePAPdf(request); | ||
| const printWindow = window.open('', '_blank'); | ||
|
|
||
| if (printWindow) { | ||
| printWindow.document.write(html); | ||
| printWindow.document.close(); | ||
|
|
||
| // Wait for content to load then trigger print | ||
| printWindow.onload = () => { | ||
| setTimeout(() => { | ||
| printWindow.print(); | ||
| }, 250); | ||
| }; | ||
| const doc = new DOMParser().parseFromString(html, 'text/html'); | ||
| const styles = doc.head.innerHTML.replace(/\bbody\b/g, `#${PDF_CONTAINER_ID}`); | ||
| const bodyContent = doc.body.innerHTML; | ||
|
|
||
| const container = document.createElement('div'); | ||
| container.id = PDF_CONTAINER_ID; | ||
| Object.assign(container.style, { | ||
| position: 'fixed', | ||
| left: '-9999px', | ||
| top: '0', | ||
| width: '210mm', | ||
| minHeight: '297mm', | ||
| background: 'white', | ||
| overflow: 'visible', | ||
| }); | ||
| container.innerHTML = `<style>${styles}</style>${bodyContent}`; | ||
| document.body.appendChild(container); | ||
|
|
||
| try { | ||
| await new Promise((r) => setTimeout(r, 100)); | ||
|
|
||
| const canvas = await html2canvas(container, { | ||
| scale: 2, | ||
| useCORS: true, | ||
| logging: false, | ||
| backgroundColor: '#ffffff', | ||
| }); | ||
|
|
||
| const pdf = new jsPDF({ format: 'a4', unit: 'mm' }); | ||
| const pageWidth = pdf.internal.pageSize.getWidth(); | ||
| const pageHeight = pdf.internal.pageSize.getHeight(); | ||
| const imgWidth = pageWidth; | ||
| const imgHeight = (canvas.height * imgWidth) / canvas.width; | ||
| const imgData = canvas.toDataURL('image/png'); | ||
|
|
||
| let heightLeft = imgHeight; | ||
| let position = 0; | ||
| let page = 0; | ||
|
|
||
| pdf.addImage(imgData, 'PNG', 0, position, imgWidth, imgHeight); | ||
| heightLeft -= pageHeight; | ||
|
|
||
| while (heightLeft > 0) { | ||
| page++; | ||
| pdf.addPage(); | ||
| position = -pageHeight * page; | ||
| pdf.addImage(imgData, 'PNG', 0, position, imgWidth, imgHeight); | ||
| heightLeft -= pageHeight; | ||
| } | ||
|
|
||
| return pdf.output('blob'); | ||
| } finally { |
There was a problem hiding this comment.
Avoid injecting <title> into the generated <style> block.
doc.head.innerHTML includes <title>, and wrapping it inside <style> produces invalid CSS, which can drop styling in the PDF. Extract only the CSS text from style tags.
🐛 Proposed fix
- const styles = doc.head.innerHTML.replace(/\bbody\b/g, `#${PDF_CONTAINER_ID}`);
+ const styles = Array.from(doc.querySelectorAll('style'))
+ .map((style) => style.innerHTML)
+ .join('\n')
+ .replace(/\bbody\b/g, `#${PDF_CONTAINER_ID}`);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| export async function generatePAPdfBlob(request: PARequest): Promise<Blob> { | |
| const html = generatePAPdf(request); | |
| const printWindow = window.open('', '_blank'); | |
| if (printWindow) { | |
| printWindow.document.write(html); | |
| printWindow.document.close(); | |
| // Wait for content to load then trigger print | |
| printWindow.onload = () => { | |
| setTimeout(() => { | |
| printWindow.print(); | |
| }, 250); | |
| }; | |
| const doc = new DOMParser().parseFromString(html, 'text/html'); | |
| const styles = doc.head.innerHTML.replace(/\bbody\b/g, `#${PDF_CONTAINER_ID}`); | |
| const bodyContent = doc.body.innerHTML; | |
| const container = document.createElement('div'); | |
| container.id = PDF_CONTAINER_ID; | |
| Object.assign(container.style, { | |
| position: 'fixed', | |
| left: '-9999px', | |
| top: '0', | |
| width: '210mm', | |
| minHeight: '297mm', | |
| background: 'white', | |
| overflow: 'visible', | |
| }); | |
| container.innerHTML = `<style>${styles}</style>${bodyContent}`; | |
| document.body.appendChild(container); | |
| try { | |
| await new Promise((r) => setTimeout(r, 100)); | |
| const canvas = await html2canvas(container, { | |
| scale: 2, | |
| useCORS: true, | |
| logging: false, | |
| backgroundColor: '#ffffff', | |
| }); | |
| const pdf = new jsPDF({ format: 'a4', unit: 'mm' }); | |
| const pageWidth = pdf.internal.pageSize.getWidth(); | |
| const pageHeight = pdf.internal.pageSize.getHeight(); | |
| const imgWidth = pageWidth; | |
| const imgHeight = (canvas.height * imgWidth) / canvas.width; | |
| const imgData = canvas.toDataURL('image/png'); | |
| let heightLeft = imgHeight; | |
| let position = 0; | |
| let page = 0; | |
| pdf.addImage(imgData, 'PNG', 0, position, imgWidth, imgHeight); | |
| heightLeft -= pageHeight; | |
| while (heightLeft > 0) { | |
| page++; | |
| pdf.addPage(); | |
| position = -pageHeight * page; | |
| pdf.addImage(imgData, 'PNG', 0, position, imgWidth, imgHeight); | |
| heightLeft -= pageHeight; | |
| } | |
| return pdf.output('blob'); | |
| } finally { | |
| export async function generatePAPdfBlob(request: PARequest): Promise<Blob> { | |
| const html = generatePAPdf(request); | |
| const doc = new DOMParser().parseFromString(html, 'text/html'); | |
| const styles = Array.from(doc.querySelectorAll('style')) | |
| .map((style) => style.innerHTML) | |
| .join('\n') | |
| .replace(/\bbody\b/g, `#${PDF_CONTAINER_ID}`); | |
| const bodyContent = doc.body.innerHTML; | |
| const container = document.createElement('div'); | |
| container.id = PDF_CONTAINER_ID; | |
| Object.assign(container.style, { | |
| position: 'fixed', | |
| left: '-9999px', | |
| top: '0', | |
| width: '210mm', | |
| minHeight: '297mm', | |
| background: 'white', | |
| overflow: 'visible', | |
| }); | |
| container.innerHTML = `<style>${styles}</style>${bodyContent}`; | |
| document.body.appendChild(container); | |
| try { | |
| await new Promise((r) => setTimeout(r, 100)); | |
| const canvas = await html2canvas(container, { | |
| scale: 2, | |
| useCORS: true, | |
| logging: false, | |
| backgroundColor: '#ffffff', | |
| }); | |
| const pdf = new jsPDF({ format: 'a4', unit: 'mm' }); | |
| const pageWidth = pdf.internal.pageSize.getWidth(); | |
| const pageHeight = pdf.internal.pageSize.getHeight(); | |
| const imgWidth = pageWidth; | |
| const imgHeight = (canvas.height * imgWidth) / canvas.width; | |
| const imgData = canvas.toDataURL('image/png'); | |
| let heightLeft = imgHeight; | |
| let position = 0; | |
| let page = 0; | |
| pdf.addImage(imgData, 'PNG', 0, position, imgWidth, imgHeight); | |
| heightLeft -= pageHeight; | |
| while (heightLeft > 0) { | |
| page++; | |
| pdf.addPage(); | |
| position = -pageHeight * page; | |
| pdf.addImage(imgData, 'PNG', 0, position, imgWidth, imgHeight); | |
| heightLeft -= pageHeight; | |
| } | |
| return pdf.output('blob'); | |
| } finally { |
🧰 Tools
🪛 ast-grep (0.40.5)
[warning] 381-381: Direct HTML content assignment detected. Modifying innerHTML, outerHTML, or using document.write with unsanitized content can lead to XSS vulnerabilities. Use secure alternatives like textContent or sanitize HTML with libraries like DOMPurify.
Context: container.innerHTML = <style>${styles}</style>${bodyContent}
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://www.dhairyashah.dev/posts/why-innerhtml-is-a-bad-idea-and-how-to-avoid-it/
- https://cwe.mitre.org/data/definitions/79.html
(unsafe-html-content-assignment)
[warning] 381-381: Direct modification of innerHTML or outerHTML properties detected. Modifying these properties with unsanitized user input can lead to XSS vulnerabilities. Use safe alternatives or sanitize content first.
Context: container.innerHTML = <style>${styles}</style>${bodyContent}
Note: [CWE-79] Improper Neutralization of Input During Web Page Generation [REFERENCES]
- https://owasp.org/www-community/xss-filter-evasion-cheatsheet
- https://cwe.mitre.org/data/definitions/79.html
(dom-content-modification)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/dashboard/src/lib/pdfGenerator.ts` around lines 365 - 418, The current
generatePAPdfBlob function builds CSS by using doc.head.innerHTML which can
include non-CSS nodes like <title>, causing invalid CSS in the injected <style>;
change the logic that computes styles (currently using
doc.head.innerHTML.replace(...)) to instead collect only CSS text from the
document's <style> elements (e.g. iterate doc.querySelectorAll('head style'),
join their .textContent, then perform the body selector replacement like
.replace(/\bbody\b/g, `#${PDF_CONTAINER_ID}`)). Keep the rest of the DOM
insertion (container.id = PDF_CONTAINER_ID, container.innerHTML) the same so
only valid CSS is injected and other head nodes (title/meta) aren’t wrapped into
the <style>.
Comment on lines
+926
to
+940
| {/* PDF Viewer Modal */} | ||
| <PdfViewerModal | ||
| isOpen={showPdfModal} | ||
| onClose={() => setShowPdfModal(false)} | ||
| request={request} | ||
| /> | ||
|
|
||
| {/* Criteria Reason Dialog */} | ||
| <CriteriaReasonDialog | ||
| isOpen={!!selectedCriterion} | ||
| onClose={() => setSelectedCriterion(null)} | ||
| met={selectedCriterion?.met ?? null} | ||
| label={selectedCriterion?.label ?? ''} | ||
| reason={selectedCriterion?.reason ?? ''} | ||
| /> |
There was a problem hiding this comment.
PDF modal should use the same effective request shown in the UI.
When reanalysis overrides criteria/confidence, the modal still receives request, which can make the PDF diverge from what the user just reviewed.
🐛 Suggested fix
- <PdfViewerModal
+ <PdfViewerModal
isOpen={showPdfModal}
onClose={() => setShowPdfModal(false)}
- request={request}
+ request={effectiveRequest}
/>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| {/* PDF Viewer Modal */} | |
| <PdfViewerModal | |
| isOpen={showPdfModal} | |
| onClose={() => setShowPdfModal(false)} | |
| request={request} | |
| /> | |
| {/* Criteria Reason Dialog */} | |
| <CriteriaReasonDialog | |
| isOpen={!!selectedCriterion} | |
| onClose={() => setSelectedCriterion(null)} | |
| met={selectedCriterion?.met ?? null} | |
| label={selectedCriterion?.label ?? ''} | |
| reason={selectedCriterion?.reason ?? ''} | |
| /> | |
| {/* PDF Viewer Modal */} | |
| <PdfViewerModal | |
| isOpen={showPdfModal} | |
| onClose={() => setShowPdfModal(false)} | |
| request={effectiveRequest} | |
| /> | |
| {/* Criteria Reason Dialog */} | |
| <CriteriaReasonDialog | |
| isOpen={!!selectedCriterion} | |
| onClose={() => setSelectedCriterion(null)} | |
| met={selectedCriterion?.met ?? null} | |
| label={selectedCriterion?.label ?? ''} | |
| reason={selectedCriterion?.reason ?? ''} | |
| /> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/dashboard/src/routes/analysis`.$transactionId.tsx around lines 926 -
940, The PDF modal is receiving the original request variable (prop request) so
it can show outdated data when reanalysis overrides criteria/confidence; update
the code so PdfViewerModal receives the effective request used by the UI (e.g.,
a computed value like effectiveRequest, requestWithOverrides, or the same
variable you render elsewhere after applying reanalysis overrides) instead of
the raw request; locate the PdfViewerModal JSX and replace the request prop with
that effective request value (ensuring that the effectiveRequest is
computed/derived earlier in the component using the same override logic as the
UI).
Comment on lines
+143
to
+148
| function formatDuration(ms: number): string { | ||
| if (ms < 60000) return `${Math.round(ms / 1000)}s`; | ||
| const mins = Math.floor(ms / 60000); | ||
| const secs = Math.round((ms % 60000) / 1000); | ||
| return secs > 0 ? `${mins}m ${secs}s` : `${mins}m`; | ||
| } |
There was a problem hiding this comment.
Avoid rounding seconds to 60 in formatDuration.
Math.round can yield 60s (e.g., 59.9s), producing 0m 60s. Flooring avoids this display glitch.
🐛 Suggested fix
- const secs = Math.round((ms % 60000) / 1000);
+ const secs = Math.floor((ms % 60000) / 1000);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/dashboard/src/routes/index.tsx` around lines 143 - 148, The
formatDuration function can produce "0m 60s" because it uses Math.round for
seconds; change the logic in formatDuration to compute seconds with Math.floor
(e.g., secs = Math.floor((ms % 60000) / 1000)) or otherwise guard against secs
=== 60 by incrementing mins and setting secs = 0, so the output never shows
"60s" (update the variables ms, mins, secs in function formatDuration
accordingly).
Comment on lines
+2
to
+4
| "ConnectionStrings": { | ||
| "authscript": "Host=localhost;Port=5433;Database=authscript;Username=postgres;Password=postgres" | ||
| }, |
There was a problem hiding this comment.
Avoid committing DB credentials in appsettings (even dev).
Hardcoded username/password in source control increases leak risk and can be reused outside local dev. Prefer user-secrets or environment variables for ConnectionStrings:authscript, and keep only a placeholder in the repo.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/gateway/Gateway.API/appsettings.Development.json` around lines 2 - 4,
Remove the hardcoded credentials from the "ConnectionStrings": { "authscript":
... } entry and replace the value with a non-secret placeholder (e.g.,
"Host=localhost;Port=5433;Database=authscript;Username={DB_USER};Password={DB_PASS}")
in appsettings.Development.json, then load the real connection string from a
secure source at runtime (environment variable or user-secrets) by wiring the
IConfiguration to read the environment key used (e.g.,
Configuration["ConnectionStrings:authscript"] or an env var like
AUTH_SCRIPT_CONNECTIONSTRING) and ensure any code that references
ConnectionStrings:authscript uses that configuration value instead of the
committed secret.
Comment on lines
+44
to
+46
|
|
||
| <!-- GraphQL --> | ||
| <PackageReference Include="HotChocolate.AspNetCore" Version="15.1.0" /> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify HotChocolate.AspNetCore 15.1.0 on NuGet
echo "=== Checking HotChocolate.AspNetCore 15.1.0 on NuGet ==="
curl -s "https://api.nuget.org/v3-flatcontainer/hotchocolate.aspnetcore/15.1.0/hotchocolate.aspnetcore.nuspec" | head -100
echo ""
echo "=== Listing recent HotChocolate.AspNetCore versions ==="
curl -s "https://api.nuget.org/v3-flatcontainer/hotchocolate.aspnetcore/index.json" | jq -r '.versions | .[-15:]' 2>/dev/null || echo "Failed to fetch version list"Repository: lvlup-sw/authscript
Length of output: 3228
🌐 Web query:
HotChocolate.AspNetCore 15.1.0 .NET 10 compatibility
💡 Result:
HotChocolate.AspNetCore 15.1.0 does not target net10.0 directly—it ships with net8.0 and net9.0 TFMs only (included in the package). However, NuGet marks net10.0 as “computed” compatible, meaning a .NET 10 app can reference it via normal .NET compatibility rules. [1]
That said, ChilliCream’s stated support for Hot Chocolate 15 is .NET 8 and .NET 9 (not .NET 10), so .NET 10 usage is “should work” compatibility rather than an explicitly supported/targeted TFM for v15. [2]
Sources: [1] NuGet package page (HotChocolate.AspNetCore 15.1.0 frameworks/compatibility), [2] ChilliCream “What’s new for Hot Chocolate 15” (supported .NET versions)
🌐 Web query:
HotChocolate.AspNetCore version 15.1.0 NuGet package security
💡 Result:
As of Feb 18, 2026, there are no published security advisories on GitHub for ChilliCream/graphql-platform (the upstream repo for Hot Chocolate), i.e., GitHub shows “There aren’t any published security advisories”. [1]
That said, HotChocolate.AspNetCore 15.1.0 is not current (NuGet shows it was last updated Mar 19, 2025). [2] The latest stable on NuGet is 15.1.12 (last updated Jan 15, 2026), and there are also newer 16.0.0 prerelease builds. [3]
Practical guidance
- If you’re using 15.1.0, prefer upgrading to ≥ 15.1.12 unless you have a compatibility reason not to. [3]
- Verify in your own solution with:
dotnet list package --vulnerable(uses known vulnerability feeds for your full dependency graph).
Sources
[1] GitHub – Security overview for ChilliCream/graphql-platform (no published advisories) (github.com)
[2] NuGet – HotChocolate.AspNetCore 15.1.0 (published/last updated Mar 19, 2025) (nuget.org)
[3] NuGet – HotChocolate.AspNetCore package page (current stable 15.1.12; prerelease versions listed) (nuget.org)
Citations:
- 1: https://github.com/ChilliCream/graphql-platform/security?utm_source=openai
- 2: https://www.nuget.org/packages/HotChocolate.AspNetCore/15.1.0?utm_source=openai
- 3: https://www.nuget.org/packages/HotChocolate.AspNetCore
Consider upgrading to HotChocolate.AspNetCore 15.1.12.
Version 15.1.0 exists on NuGet and will function on .NET 10 through computed compatibility. However, the package explicitly targets only .NET 8 and 9, and ChilliCream's official support for Hot Chocolate 15 does not include .NET 10. A newer patch version (15.1.12, released Jan 15, 2026) is available and recommended for optimal compatibility and latest bug fixes. No security advisories are reported for this package.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/gateway/Gateway.API/Gateway.API.csproj` around lines 44 - 46, Update the
HotChocolate.AspNetCore package reference in Gateway.API.csproj from
Version="15.1.0" to Version="15.1.12" to pick up the latest 15.x patch fixes and
improved .NET 10 compatibility; locate the <PackageReference
Include="HotChocolate.AspNetCore" Version="15.1.0" /> entry and change the
Version attribute to "15.1.12", then restore/update NuGet packages and run the
project/tests to verify no regressions.
Comment on lines
+1
to
+13
| namespace Gateway.API.GraphQL.Inputs; | ||
|
|
||
| public sealed record UpdatePARequestInput( | ||
| string Id, | ||
| string? Diagnosis, | ||
| string? DiagnosisCode, | ||
| string? ServiceDate, | ||
| string? PlaceOfService, | ||
| string? ClinicalSummary, | ||
| IReadOnlyList<CriterionInput>? Criteria | ||
| ); | ||
|
|
||
| public sealed record CriterionInput(bool? Met, string Label, string? Reason = null); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Split CriterionInput into its own file (or make it non-public).
This file declares two public types, which violates the one-public-type-per-file rule. As per coding guidelines, “SOLID: One public type per file, Strategy over switches, constructor injection.”
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/gateway/Gateway.API/GraphQL/Inputs/UpdatePARequestInput.cs` around lines
1 - 13, The file currently declares two public types (UpdatePARequestInput and
CriterionInput), violating the one-public-type-per-file rule; fix by either
moving CriterionInput into its own file as a public sealed record named
CriterionInput(bool? Met, string Label, string? Reason = null) or change its
accessibility to non-public (e.g., internal sealed record CriterionInput(...))
so only UpdatePARequestInput remains public; update any references/imports
accordingly to compile.
Comment on lines
+137
to
181
| async def extract_evidence( | ||
| clinical_bundle: ClinicalBundle, | ||
| policy: dict[str, Any], | ||
| ) -> list[EvidenceItem]: | ||
| """ | ||
| Extract evidence from clinical bundle using LLM to evaluate policy criteria. | ||
|
|
||
| Evaluates criteria concurrently with a configurable concurrency limit. | ||
|
|
||
| Args: | ||
| clinical_bundle: FHIR clinical data bundle | ||
| policy: Policy definition with criteria | ||
|
|
||
| Returns: | ||
| List of evidence items, one per policy criterion | ||
| """ | ||
| criteria = policy.get("criteria", []) | ||
| if not criteria: | ||
| return [] | ||
|
|
||
| clinical_summary = _build_clinical_summary(clinical_bundle) | ||
| semaphore = _get_llm_semaphore() | ||
|
|
||
| results = await asyncio.gather( | ||
| *[_bounded_evaluate(c, clinical_summary, semaphore) for c in criteria], | ||
| return_exceptions=True, | ||
| ) | ||
|
|
||
| evidence_items: list[EvidenceItem] = [] | ||
| for i, result in enumerate(results): | ||
| if isinstance(result, BaseException): | ||
| criterion_id = criteria[i].get("id", "unknown") | ||
| logger.error("Criterion %s evaluation failed: %s", criterion_id, result) | ||
| evidence_items.append( | ||
| EvidenceItem( | ||
| criterion_id=criterion_id, | ||
| status="UNCLEAR", | ||
| evidence=f"Evaluation error: {result}", | ||
| source="LLM analysis", | ||
| confidence=0.0, | ||
| ) | ||
| ) | ||
| ) | ||
| else: | ||
| evidence_items.append(result) | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "evidence_extractor.py" -path "*/intelligence/*"Repository: lvlup-sw/authscript
Length of output: 118
🏁 Script executed:
cd apps/intelligence 2>/dev/null && find . -name "evidence_extractor.py" || find . -name "evidence_extractor.py" -type fRepository: lvlup-sw/authscript
Length of output: 100
🏁 Script executed:
find . -type f -name "evidence_extractor.py"Repository: lvlup-sw/authscript
Length of output: 118
🏁 Script executed:
sed -n '137,181p' ./apps/intelligence/src/reasoning/evidence_extractor.pyRepository: lvlup-sw/authscript
Length of output: 1527
🏁 Script executed:
head -50 ./apps/intelligence/src/reasoning/evidence_extractor.pyRepository: lvlup-sw/authscript
Length of output: 1373
🏁 Script executed:
grep -n "import asyncio\|from asyncio\|CancelledError" ./apps/intelligence/src/reasoning/evidence_extractor.pyRepository: lvlup-sw/authscript
Length of output: 79
🏁 Script executed:
python3 << 'EOF'
import asyncio
# Verify CancelledError hierarchy
print("asyncio.CancelledError bases:", asyncio.CancelledError.__bases__)
print("Is CancelledError an Exception?", issubclass(asyncio.CancelledError, Exception))
print("Is CancelledError a BaseException?", issubclass(asyncio.CancelledError, BaseException))
# In Python 3.8+, CancelledError is Exception, but we should still handle it specially
# as per asyncio best practices for gather with return_exceptions=True
EOFRepository: lvlup-sw/authscript
Length of output: 197
🏁 Script executed:
grep -A 5 -B 5 "return_exceptions" ./apps/intelligence/src/reasoning/evidence_extractor.pyRepository: lvlup-sw/authscript
Length of output: 450
🏁 Script executed:
cat -n ./apps/intelligence/src/reasoning/evidence_extractor.py | sed -n '155,175p'Repository: lvlup-sw/authscript
Length of output: 1014
🏁 Script executed:
python3 << 'EOF'
import asyncio
import sys
# Test scenario: what happens with return_exceptions=True and CancelledError
print(f"Python version: {sys.version}")
print()
print("Key facts about asyncio.CancelledError:")
print("- In Python 3.8+, CancelledError inherits directly from BaseException")
print("- With gather(return_exceptions=True), ALL exceptions are returned in results list")
print("- Including CancelledError")
print()
print("The concern:")
print("- isinstance(result, BaseException) will match CancelledError")
print("- Converting it to an UNCLEAR status prevents cancellation propagation")
print("- This breaks task cancellation semantics")
print()
print("The fix:")
print("- Check for asyncio.CancelledError first and re-raise")
print("- Use isinstance(result, Exception) instead of BaseException")
print("- This lets CancelledError propagate while handling other errors")
EOFRepository: lvlup-sw/authscript
Length of output: 728
Re-raise asyncio.CancelledError to preserve cancellation semantics.
With return_exceptions=True, asyncio.gather() returns exceptions as values in the results list, including asyncio.CancelledError. The current code treats all BaseException instances as recoverable errors, converting cancellations into "UNCLEAR" items. This prevents request cancellations from propagating properly.
Check for asyncio.CancelledError first and re-raise it, or use isinstance(result, Exception) instead of BaseException to let cancellations propagate correctly:
Suggested fix
for i, result in enumerate(results):
+ if isinstance(result, asyncio.CancelledError):
+ raise result
- if isinstance(result, BaseException):
+ if isinstance(result, Exception):
criterion_id = criteria[i].get("id", "unknown")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/intelligence/src/reasoning/evidence_extractor.py` around lines 137 -
181, The loop in extract_evidence treats any BaseException returned from
asyncio.gather as a recoverable error and converts cancellations into "UNCLEAR"
results; change the handling to let asyncio.CancelledError propagate by checking
for asyncio.CancelledError first and re-raising it (or by only treating
instances of Exception as recoverable), i.e., in the results processing inside
extract_evidence (which handles values from _bounded_evaluate) detect
asyncio.CancelledError and raise it, otherwise log and convert other exceptions
into an EvidenceItem as before.
- Add dashboard as ViteApp resource in AppHost, wired to gateway/intelligence - Swap deprecated Aspire.Hosting.NodeJs for Aspire.Hosting.JavaScript 13.1.0 - Route GraphQL requests through Vite proxy in dev to avoid cross-origin issues - Relax Node engine constraint to >=20.0.0 (remove upper bound) - Remove platform-specific optionalDependencies; regenerate lock file with cross-platform native binding entries - Add NVM symlink note for Aspire compatibility Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Restore approve/deny mutations and connection status hook in graphqlService - Fix graphql-request mock type cast in test - Fix ruff lint: line length in config.py, import sorting and long lines in test_evidence_extractor.py - Sync generated schemas to resolve drift Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Use explicit default= keyword in Field() for mypy compatibility - Add type parameters to dict annotation in OpenAIProvider Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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
2 participants
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
Optimizes the intelligence service from 15-35s to 3-5s per request by parallelizing LLM criterion evaluation, and establishes clean abstraction boundaries in the gateway with an IDataService interface. Consolidates dual token systems in the dashboard and adds approve/deny state transitions.
Changes
Test Plan
All three service test suites pass: Python 27 passed, .NET 380 passed (3 skipped), TypeScript 206 passed. New tests cover parallel extraction, client pooling, error handling, IDataService operations, and auth token consolidation.
Results: Tests 613 ✓ · Build 0 errors
Design: dashboard-backend-optimization.md