More FHIR capabilities#59
Open
pauljohanneskraft wants to merge 35 commits into
Open
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughAdds extensive FHIR questionnaire features: enableWhen evaluation, page-aware navigation, question simplification, hierarchical responses, numeric/decimal validation, decimal answer support, CallSession section-transition handlers, and many unit tests; also small config/constant and persistence changes. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CallSession
participant Engine as FHIRQuestionnaireEngine
participant Store as ResponseStore
User->>CallSession: submit answer
CallSession->>Engine: answerQuestion(linkId, answer)
Engine-->>Engine: validateAnswer() / update response (hierarchical)
Engine->>CallSession: nextQuestionString(includeAllQuestions)
alt next section available
CallSession->>CallSession: handleNextSectionAvailable(initialQuestion, systemMessage)
CallSession->>Store: create response record
else all sections complete
CallSession->>Engine: hierarchicalResponse()
CallSession->>Store: save final response
end
CallSession->>User: emit function output / system message
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
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 |
pauljohanneskraft
changed the title
pauljohanneskraft
changed the title
pauljohanneskraft
force-pushed
the
feb-improvements-3
branch
from
9a6bf32 to
026f806
Compare
pauljohanneskraft
force-pushed
the
feb-improvements-3
branch
from
07ec1f6 to
79db192
Compare
pauljohanneskraft
force-pushed
the
feb-improvements-4
branch
2 times, most recently
from
f53a288 to
a365a5e
Compare
pauljohanneskraft
force-pushed
the
feb-improvements-3
branch
from
b550772 to
99f6a40
Compare
pauljohanneskraft
force-pushed
the
feb-improvements-4
branch
from
a365a5e to
2c119e7
Compare
pauljohanneskraft
force-pushed
the
feb-improvements-4
branch
from
2c119e7 to
250c811
Compare
pauljohanneskraft
changed the title
Contributor
There was a problem hiding this comment.
Pull request overview
This PR expands the app’s FHIR Questionnaire support by adding enableWhen/enableBehavior evaluation, richer question “simplification” metadata, enhanced navigation (including page-aware behavior), additional answer decoding/handling, and new unit tests to cover the added behaviors.
Changes:
- Add new
FHIRQuestionnaireEngineextensions for enableWhen evaluation, page-aware navigation, simplification, and basic answer constraint validation. - Update call flow/tooling to use
nextQuestionString, accept decimal answers, and update section placeholder naming. - Add a substantial new test suite covering navigation, simplification, enableWhen, response args encoding/decoding, and validation.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| Tests/AppTests/AppTests.swift | Switches to shared withTestApp helper for consistent app lifecycle in tests. |
| Tests/AppTests/ValidationTests.swift | Adds validation-focused tests for numeric bounds, error types, and tool-call behavior. |
| Tests/AppTests/SimplificationTests.swift | Adds tests for simplification outputs (bounds, units, answer options, initial values, readOnly). |
| Tests/AppTests/QuestionnaireResponseArgsTests.swift | Adds tests for encoding/decoding expanded answer argument types (including decimals). |
| Tests/AppTests/NavigationTests.swift | Adds tests for next-question selection, progress, skipping display/hidden, page-group detection, overwrites. |
| Tests/AppTests/EnableWhenTests.swift | Adds tests for enableWhen operators, enableBehavior any/all, and hierarchical response generation. |
| Tests/AppTests/Helpers/QuestionnaireTestHelpers.swift | Introduces shared test helpers and FHIR builder utilities to assemble items/conditions. |
| Sources/App/constants.swift | Renames section placeholder constant to sectionNumberPlaceholder. |
| Sources/App/Services/Questionnaire/SimplifiedQuestion.swift | Expands simplified question payload (min/max as Double, open-choice, readOnly, initial, maxLength, unit). |
| Sources/App/Services/Questionnaire/QuestionnaireResponseStore.swift | Adjusts persistence write options when saving responses. |
| Sources/App/Services/Questionnaire/QuestionnaireResponseArgs.swift | Adds a decimal answer case and updates Codable implementation accordingly. |
| Sources/App/Services/Questionnaire/FHIRQuestionnaireEngine.swift | Adds answer validation error type, hides/display filtering, hierarchical response saving, and API renames. |
| Sources/App/Services/Questionnaire/FHIRQuestionnaireEngine+Simplification.swift | New: simplification helpers, bounds/unit extraction, and constraint validation. |
| Sources/App/Services/Questionnaire/FHIRQuestionnaireEngine+Navigation.swift | New: enable-aware and page-aware next-question selection and “allQuestions” behavior. |
| Sources/App/Services/Questionnaire/FHIRQuestionnaireEngine+EnableWhen.swift | New: enableWhen/enableBehavior evaluation, initial prepopulation, and hierarchy building helpers. |
| Sources/App/Services/Questionnaire/EngageHF/EngageHFSections.swift | Updates instructions to use the renamed section placeholder and adjusts swiftlint config. |
| Sources/App/Services/Questionnaire/CallFlowCoordinator.swift | Uses nextQuestionString and updated placeholder replacement for section numbering. |
| Sources/App/Services/Call/CallSession.swift | Adds decimal answer handling and uses nextQuestionString; exposes helpers used by section-transition extension. |
| Sources/App/Services/Call/CallSession+SectionTransition.swift | New: refactors questionnaire completion/transition and error handling out of CallSession.swift. |
| Sources/App/Resources/sessionConfig.json | Updates tool description text to document expected formats for more FHIR answer types. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| /// Persist the current response to disk. | ||
| func save() { | ||
| store.saveResponse(response) | ||
| store.saveResponse(hierarchicalResponse()) |
There was a problem hiding this comment.
save() now persists hierarchicalResponse(), but the engine’s in-memory logic (answeredIds lookup, overwrite-by-linkId, enableWhen evaluation, etc.) assumes response.item is a flat list. On the next launch, store.loadResponse() will load the hierarchical structure, and methods like response.item?.firstIndex(where:) / compactMap will miss nested answers, causing duplicates and incorrect navigation/enableWhen behavior. Suggest flattening the loaded response into the engine’s internal representation (e.g., response.item = Self.flattenResponseItems(response.item ?? [])) or continuing to persist the flat response and only generating the hierarchical form when exporting.
Suggested change
| store.saveResponse(hierarchicalResponse()) | |
| // Persist the internal flat response; hierarchical nesting is derived on demand. | |
| store.saveResponse(response) |
| case .string(let string): | ||
| return compareStringAnswer(responseAnswer, expected: string, comparison: comparison) | ||
| default: | ||
| return true |
There was a problem hiding this comment.
compareAnswerValues returns true for any unhandled enableWhen answer type (default: return true). That makes unsupported conditions evaluate as satisfied and can incorrectly enable questions that should remain disabled. Returning false (or explicitly handling additional FHIR types) would be safer and aligns with the idea that an unmet/unknown condition should not enable an item.
Suggested change
| return true | |
| return false |
| let systemMessage = Constants.feedback( | ||
| content: feedback ?? "Feedback failed to be retrieved." | ||
| ) | ||
| try await updateSession(systemMessage: systemMessage) |
There was a problem hiding this comment.
handleAllSectionsComplete does not send a function_call_output for the save_response tool call. For tool-calling flows, each function call generally must be completed with a corresponding output; otherwise the client/model may remain waiting for tool output even though response.create is sent. Consider sending an explicit completion output (e.g., a final message or empty output) before sendResponseCreate().
Suggested change
| try await updateSession(systemMessage: systemMessage) | |
| try await updateSession(systemMessage: systemMessage) | |
| try await sendFunctionOutput(callId: response.callId ?? "", output: "") |
| case .string(let val): answerItem.value = .string(val) | ||
| case .integer(let val): answerItem.value = .integer(val) | ||
| case .decimal(let val): answerItem.value = .decimal(val) | ||
| case .boolean(let val): answerItem.value = .boolean(val) |
There was a problem hiding this comment.
prePopulateInitialValues skips .coding initial values (default: continue), but initialValueString/simplification explicitly supports .coding. This means a questionnaire can advertise an initial coding value to clients while the engine doesn't actually pre-populate the response with it. Consider handling .coding here (and choosing a consistent representation for choice/open-choice answers, e.g., store the code string or a Coding value).
Suggested change
| case .boolean(let val): answerItem.value = .boolean(val) | |
| case .boolean(let val): answerItem.value = .boolean(val) | |
| case .coding(let val): answerItem.value = .coding(val) |
Comment on lines
195
to
+201
| /// Record an answer for a given linkId. | ||
| func answerQuestion<T>(linkId: String, answer: T) throws { | ||
| // Validate the answer against questionnaire constraints before saving | ||
| if let validationError = validateAnswer(linkId: linkId, answer: answer) { | ||
| throw validationError | ||
| } | ||
|
|
There was a problem hiding this comment.
answerQuestion now validates bounds via validateAnswer, but neither validation nor extractAnswerItemValue enforce that the provided Swift type matches the Questionnaire item’s declared type (e.g., a Double can be recorded for an integer item, and a String can be recorded for numeric items). This can generate QuestionnaireResponses that are not type-consistent with the Questionnaire definition. Consider adding a type-compatibility check (based on questionnaireItem.type) and rejecting/coercing inputs that don’t match (e.g., only allow .decimal when the item type is decimal, require integral values for integer).
Comment on lines
+41
to
+54
| /// Validation errors returned when an answer fails constraint checks. | ||
| enum AnswerValidationError: Error, CustomStringConvertible { | ||
| case belowMinimum(Double) | ||
| case aboveMaximum(Double) | ||
| case exceedsMaxLength(Int) | ||
|
|
||
| var description: String { | ||
| switch self { | ||
| case .belowMinimum(let min): return "Value must be at least \(min)" | ||
| case .aboveMaximum(let max): return "Value must be at most \(max)" | ||
| case .exceedsMaxLength(let max): return "Text must be at most \(max) characters" | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
AnswerValidationError is thrown from answerQuestion, but it only conforms to CustomStringConvertible. Call sites that surface error.localizedDescription (e.g., tool output) will likely emit a generic message instead of these descriptions. Consider conforming to LocalizedError and returning these strings from errorDescription (or ensure callers use String(describing:)).
| try await sendFunctionOutput( | ||
| callId: response.callId ?? "", | ||
| output: "Failed to process questionnaire" | ||
| ) |
There was a problem hiding this comment.
handleProcessingError sends a function_call_output but never calls sendResponseCreate(). In the current call flow, this leaves the session without a follow-up response.create, so the model may not continue after an error. Consider mirroring the other code paths by calling sendResponseCreate() after sending the error output (and/or updating the session instructions as needed).
Suggested change
| ) | |
| ) | |
| try await sendResponseCreate() |
Comment on lines
30
to
45
| @@ -41,7 +41,7 @@ | |||
| { "type": "number" }, | |||
| { "type": "null" } | |||
| ], | |||
| "description": "The patient's answer. A `null` value must ONLY be used if the patient explicitly asks to skip the question or clearly states they do not have the information. Never use `null` because of silence, filler words, or unclear responses." | |||
| "description": "The patient's answer. Use a string for choice codes, free text, dates, and booleans. Use a number (integer or decimal) for numeric questions. A `null` value must ONLY be used if the patient explicitly asks to skip the question or clearly states they do not have the information. Never use `null` because of silence, filler words, or unclear responses." | |||
| } | |||
There was a problem hiding this comment.
The updated tool description instructs callers to provide date answers as YYYY-MM-DD strings and boolean answers as 'true'/'false' strings, but the server-side parsing currently treats all strings as .string FHIR answers (and answerQuestion only produces a boolean answer when it receives a Swift Bool). Either align this description with what the backend actually accepts today, or add backend parsing that converts these string forms into the appropriate FHIR value types based on the questionnaire item type.
Comment on lines
+92
to
+102
| // MARK: - MaxLength validation | ||
|
|
||
| @Test("String answer exceeding maxLength is rejected") | ||
| @MainActor | ||
| func stringExceedsMaxLength() async throws { | ||
| try await withTestApp { app in | ||
| let engine = try makeTestEngine(resourceName: "vitalSigns", app: app) | ||
|
|
||
| // We validate a builder-made item with maxLength through validateAnswer | ||
| // Since vitalSigns doesn't have string items, we test the validateAnswer | ||
| // directly with an item that doesn't exist — should return nil (no item found) |
There was a problem hiding this comment.
This test is labeled as maxLength validation, but it calls validateAnswer with a linkId that doesn't exist in the questionnaire and then expects nil. That doesn't exercise the maxLength logic at all, so it can pass even if maxLength validation is broken. Consider adding (or referencing) a questionnaire fixture that includes a string/text item with maxLength, and assert that validateAnswer returns .exceedsMaxLength when the answer is too long.
Suggested change
| // MARK: - MaxLength validation | |
| @Test("String answer exceeding maxLength is rejected") | |
| @MainActor | |
| func stringExceedsMaxLength() async throws { | |
| try await withTestApp { app in | |
| let engine = try makeTestEngine(resourceName: "vitalSigns", app: app) | |
| // We validate a builder-made item with maxLength through validateAnswer | |
| // Since vitalSigns doesn't have string items, we test the validateAnswer | |
| // directly with an item that doesn't exist — should return nil (no item found) | |
| // MARK: - Nonexistent linkId validation | |
| @Test("validateAnswer returns nil for nonexistent linkId") | |
| @MainActor | |
| func validateAnswerNonexistentLinkIdReturnsNil() async throws { | |
| try await withTestApp { app in | |
| let engine = try makeTestEngine(resourceName: "vitalSigns", app: app) | |
| // vitalSigns doesn't contain an item with linkId "nonexistent" | |
| // validateAnswer should return nil when no matching item is found |
| let jsonData = try encoder.encode(response) | ||
| let dataToWrite = try encryptIfNeeded(jsonData, logger: logger) | ||
| try dataToWrite.write(to: fileURL(phoneNumber), options: .atomic) | ||
| try dataToWrite.write(to: fileURL(phoneNumber)) |
There was a problem hiding this comment.
saveResponse no longer writes with .atomic. Without atomic writes, a crash/kill mid-write can leave a truncated/corrupted QuestionnaireResponse on disk (especially problematic with encryption). Consider restoring .atomic (or writing to a temp file + replace) so persisted responses are always valid.
Suggested change
| try dataToWrite.write(to: fileURL(phoneNumber)) | |
| try dataToWrite.write(to: fileURL(phoneNumber), options: .atomic) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/App/Services/Call/CallSession.swift (1)
190-194:⚠️ Potential issue | 🟠 MajorDon't treat serialization failure as questionnaire completion.
nextQuestionStringreturnsnilboth when there is no next item and when encoding fails. This branch will advance/end the questionnaire on serialization errors instead of surfacing them.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/App/Services/Call/CallSession.swift` around lines 190 - 194, The code treats a nil from engine.nextQuestionString as “no next question,” but nextQuestionString currently returns nil for both “no next item” and “serialization/encoding failure,” which causes questionnaire completion to be triggered on errors; update nextQuestionString to either throw on serialization errors or return a discriminating enum/Result (e.g., .next(String) | .none | .error(Error)), then change this call in CallSession.swift to await and handle the error case explicitly (catch the thrown error or switch the Result/enum) — only call handleQuestionnaireComplete(response:) when you have an explicit “no next” result and propagate/log serialization errors instead of treating them as completion; reference engine.nextQuestionString, handleNextQuestionAvailable(nextQuestion:response:), and handleQuestionnaireComplete(response:) when making the change.
🧹 Nitpick comments (4)
Sources/App/Services/Questionnaire/FHIRQuestionnaireEngine+EnableWhen.swift (1)
110-123: Potential precision loss when comparing decimals as Doubles.Converting
FHIRDecimaltoDoubleviaNSDecimalNumber.doubleValuecan lose precision for values with many significant digits. For medical/clinical data, consider usingDecimalcomparison directly if precision is critical.♻️ Decimal-native comparison
func compareDecimalAnswer( _ responseAnswer: QuestionnaireResponseItemAnswer, expected: FHIRPrimitive<FHIRDecimal>, comparison: QuestionnaireItemOperator ) -> Bool { guard let responseValue = responseAnswer.value, case .decimal(let responseDecimal) = responseValue else { return false } - let lhs = NSDecimalNumber(decimal: responseDecimal.value?.decimal ?? 0).doubleValue - let rhs = NSDecimalNumber(decimal: expected.value?.decimal ?? 0).doubleValue - return compareNumbers(lhs, rhs, comparison: comparison) + let lhs = responseDecimal.value?.decimal ?? Decimal(0) + let rhs = expected.value?.decimal ?? Decimal(0) + return compareDecimals(lhs, rhs, comparison: comparison) } + + private func compareDecimals( + _ lhs: Decimal, _ rhs: Decimal, comparison: QuestionnaireItemOperator + ) -> Bool { + switch comparison { + case .equal: return lhs == rhs + case .notEqual: return lhs != rhs + case .greaterThan: return lhs > rhs + case .lessThan: return lhs < rhs + case .greaterThanOrEqual: return lhs >= rhs + case .lessThanOrEqual: return lhs <= rhs + default: return false + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/App/Services/Questionnaire/FHIRQuestionnaireEngine`+EnableWhen.swift around lines 110 - 123, The compareDecimalAnswer function currently converts FHIRDecimal values to Double (via NSDecimalNumber.doubleValue) which can lose precision; update it to compare using Decimal (or NSDecimalNumber.compare) directly: extract responseDecimal.value?.decimal and expected.value?.decimal as Decimal, then perform a decimal-native comparison (or add/overload compareNumbers to accept Decimals) and use the resulting comparison to evaluate the QuestionnaireItemOperator. Keep references to compareDecimalAnswer and compareNumbers (or introduce compareNumbersDecimal/NSDecimalNumber.compare) so reviewers can find the change.Tests/AppTests/EnableWhenTests.swift (2)
94-107: Verify coding comparison test relies on internal code mapping.This test answers with
"much-worse"but expects equality withanswerCoding: "1". This assumes the questionnaire's code mapping translates"much-worse"→"1"internally. Consider adding a comment clarifying this dependency on theq17resource's answer options, or alternatively test with the raw code value directly to make the test self-documenting.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/AppTests/EnableWhenTests.swift` around lines 94 - 107, The test enableWhenCodingEqual relies on an implicit mapping in the "q17" resource that maps the displayed answer "much-worse" to coding "1"; update the test to make this dependency explicit by either adding a short inline comment above the FHIRBuilder.enableWhen(...) and engine.answerQuestion(...) calls explaining that "much-worse" maps to coding "1" in resource "q17", or change the test to answer with the raw code value (e.g., use the coded value instead of "much-worse") so the equality check in engine.evaluateCondition(condition) is self-documenting; reference the test function enableWhenCodingEqual, the FHIRBuilder.enableWhen(...) call, and engine.answerQuestion(linkId: "wellbeing-comparison", answer: ...) when making the change.
253-278: Hierarchical response test makes assumptions about vitalSigns structure.The test asserts
topItems.count == 3and specific linkIds at fixed indices. If thevitalSignsquestionnaire resource is modified, this test will break silently. Consider either:
- Adding a brief comment documenting the expected structure
- Using more flexible assertions (e.g., finding items by linkId rather than by index)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Tests/AppTests/EnableWhenTests.swift` around lines 253 - 278, The test hierarchicalResponseStructure currently assumes a fixed order in engine.hierarchicalResponse() by asserting topItems.count == 3 and checking specific indices; make it robust by locating items by linkId instead of relying on array indices (e.g., find item where item.linkId.value?.string == "blood-pressure-group" and assert its children count, find items for "heart-rate" and "weight" and assert they have answers), or if you prefer to keep the ordering assertions, add a concise comment describing the expected vitalSigns structure so future editors know the dependency; update the assertions in hierarchicalResponseStructure to use search-by-linkId on topItems and validate counts/answers accordingly.Sources/App/Services/Questionnaire/FHIRQuestionnaireEngine+Navigation.swift (1)
34-49: Consider extracting the editable item filter predicate.The logic for determining if an item is editable (not readOnly, not answered) is duplicated multiple times in this method and in
unansweredEnabledItems. Consider extracting a helper predicate to reduce duplication.♻️ Example helper extraction
private func isEditableAndUnanswered(_ item: QuestionnaireItem, answeredIds: Set<String>) -> Bool { guard let linkId = item.linkId.value?.string else { return false } let isReadOnly = item.readOnly?.value?.bool ?? false return !isReadOnly && !answeredIds.contains(linkId) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/App/Services/Questionnaire/FHIRQuestionnaireEngine`+Navigation.swift around lines 34 - 49, The closures that check editable/unanswered items are duplicated (used in the enabledItems.first chains and in unansweredEnabledItems); extract a helper like isEditableAndUnanswered(_ item: QuestionnaireItem, answeredIds: Set<String>) -> Bool that performs the linkId unwrap, readOnly check, and answeredIds containment test, then replace the duplicated predicates in enabledItems.first and unansweredEnabledItems with calls to this helper (for the required-first branch, combine the helper with the required check: isEditableAndUnanswered(item, answeredIds) && (item.required?.value?.bool ?? false)).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/App/Services/Call/CallSession`+SectionTransition.swift:
- Around line 54-59: The error path in handleProcessingError currently logs and
calls sendFunctionOutput but never sends the final response.create message;
after calling sendFunctionOutput (using the same callId from response.callId ??
""), also invoke sendResponseCreate(...) with the failure output so the model
receives the tool result and the call completes; ensure you use the same
OpenAIResponse/response context and mirror other code paths that call
sendResponseCreate after sendFunctionOutput.
- Around line 45-49: In handleAllSectionsComplete, stop substituting a
placeholder string when feedback generation fails; call
Constants.feedback(content: feedback) with feedback (which may be nil) so the
helper can use its built-in failure template—update the systemMessage
construction in handleAllSectionsComplete to pass the optional returned from
coordinator.generateFeedback() directly to Constants.feedback.
- Around line 15-24: advanceToNextSection() may land on sections with no
answerable first question because nextQuestionString(includeAllQuestions:) can
return nil; update the logic around calling advanceToNextSection(),
nextQuestionString(...), sectionSystemMessage(...), and
handleNextSectionAvailable(...) to skip over any advanced sections where
initialQuestion is nil or sectionSystemMessage returns nil: loop calling
coordinator.advanceToNextSection() until it returns nil or you obtain a non-nil
initialQuestion and systemMessage, and only then call
handleNextSectionAvailable(...) and create/update the response; if
advanceToNextSection() returns nil, bail out without changing the
session/response. Ensure you reference the existing methods
coordinator.advanceToNextSection(), nextQuestionString(includeAllQuestions:),
coordinator.sectionSystemMessage(for:initialQuestion:), and
handleNextSectionAvailable(...) when making the change.
In `@Sources/App/Services/Questionnaire/FHIRQuestionnaireEngine.swift`:
- Around line 254-257: The save() call currently persists
hierarchicalResponse(), which breaks the flat-item assumptions in
loadResponse(), answerQuestion(), updateFinishedState(), and scoring; change
persistence to store the flat response and generate hierarchy on-demand: modify
save() to persist the flattened response (use the existing
flattenResponseItems() utility or a flatResponse() helper) and keep
hierarchicalResponse() as a transient generator, or alternatively ensure
loadResponse() calls flattenResponseItems() immediately after deserialization so
response.item remains flat for answerQuestion(), updateFinishedState(), and
scoring; update references to hierarchicalResponse(), loadResponse(), and
flattenResponseItems() accordingly.
In `@Sources/App/Services/Questionnaire/FHIRQuestionnaireEngine`+EnableWhen.swift:
- Around line 155-163: The compareStrings function currently falls back to
equality in its default case, which can silently succeed for unsupported
operators (e.g., greaterThan); update the default branch in compareStrings(_:_:,
comparison:) to return false instead of lhs == rhs so unsupported string
operators fail safely (keep the existing .equal and .notEqual handling intact).
- Around line 69-71: The switch's default currently returns true in
FHIRQuestionnaireEngine+EnableWhen (the enableWhen evaluation routine) causing
unsupported answer types to be treated as enabled; change the default branch to
return false and emit a warning/log (use the engine's logger) that includes the
unsupported answer type/value (e.g., the answer.type or answerValue variable) so
misconfigured enableWhen rules are visible; update any unit tests for the
enableWhen evaluator (e.g., tests referencing evaluateEnableWhen/isEnabled) to
expect false for unsupported types.
In `@Sources/App/Services/Questionnaire/FHIRQuestionnaireEngine`+Navigation.swift:
- Around line 103-129: Update the docstring on currentPageQuestions to
explicitly state that pages with one or fewer unanswered, non-readOnly questions
are intentionally excluded (the function returns [] when pageQuestions.count <=
1) to avoid duplicating the current question in allQuestions; mention the
criteria used (non-readOnly, enabledItems, unansweredIds, and page membership
via findPageGroup and flattenItems). Also add a unit test that constructs a
Questionnaire page with exactly one unanswered non-readOnly question and asserts
currentPageQuestions returns an empty array, plus a test with two unanswered
questions that asserts the simplified questions are returned.
In
`@Sources/App/Services/Questionnaire/FHIRQuestionnaireEngine`+Simplification.swift:
- Around line 68-83: The closure currently fabricates empty/zero options by
defaulting missing primitives; update the options.compactMap logic (the closure
that returns SimplifiedAnswerOption) to only produce a SimplifiedAnswerOption
when the underlying FHIR primitive actually exists and is non-empty: for the
.coding branch require coding.display?.value?.string to be non-nil/non-empty
before calling Self.descriptiveCode and Self.extractNote; for .integer require
intVal.value?.integer to be non-nil and convert that integer to a string instead
of defaulting to 0; for .string require strVal.value?.string to be
non-nil/non-empty before computing code via Self.descriptiveCode; otherwise
return nil so incomplete FHIR option entries are skipped rather than emitting
bogus options.
- Around line 15-28: The helper initialValueString(_: QuestionnaireItemInitial)
currently ignores date-style initials; update the switch in initialValueString
to handle date (and dateTime if applicable) cases by extracting and returning
the string representation of the date value from the QuestionnaireItemInitial
(e.g., use the wrapped .date/.dateTime value on the associated value container
similar to how .string/.integer are handled). Modify the switch to include case
.date(let d): return d.value?.date?.string (and a similar case for .dateTime if
your model has it) so prefilled date questions populate
SimplifiedQuestion.initialValue.
- Around line 132-145: The numeric-range check in
FHIRQuestionnaireEngine+Simplification.swift currently only handles Int/Double
and thus skips numeric validation when answers are numeric strings; update the
validation around the existing numeric block that uses
simplified.minValue/simplified.maxValue and answer so that if answer is a String
you attempt to parse it to a Double (e.g. via Double(stringValue)) and then
apply the same belowMinimum/aboveMaximum checks, while preserving the separate
string maxLength check (exceedsMaxLength) for non-numeric strings; reference
simplified.minValue, simplified.maxValue, answer, and the
.belowMinimum/.aboveMaximum/.exceedsMaxLength result cases when making the
change.
- Around line 92-108: The loop that reads extensions in
FHIRQuestionnaireEngine+Simplification.swift is defaulting missing primitive
values to zero (using val.value?.integer ?? 0 and val.value?.decimal ?? 0),
which incorrectly treats absent min/max as a real bound; update the handling
inside the cases for .integer and .decimal so you only assign to
minValue/maxValue when val.value?.integer or val.value?.decimal is non-nil
(e.g., unwrap the optional and convert to Double/NSDecimalNumber only when
present), leaving minValue/maxValue as nil when the primitive is missing; keep
the checks against Self.minValueURL and Self.maxValueURL and the existing
ext.value pattern matching.
In `@Sources/App/Services/Questionnaire/QuestionnaireResponseStore.swift`:
- Line 98: The write removed the .atomic option so writes in
QuestionnaireResponseStore (the call that does dataToWrite.write(to:
fileURL(phoneNumber))) are no longer atomic; restore atomicity by using the Data
write overload that accepts FileHandle/NSDataWritingOptions and pass the .atomic
option (e.g., use dataToWrite.write(to: fileURL(phoneNumber), options: .atomic))
in the method that persists responses in QuestionnaireResponseStore so
partial/corrupt files cannot be left behind.
In `@Tests/AppTests/QuestionnaireResponseArgsTests.swift`:
- Around line 42-51: The test decodeIntegerLookingDecimal currently uses JSON
with integer 5, so it doesn't exercise the decimal-to-integer decoding path;
update the test in function decodeIntegerLookingDecimal to use a JSON value with
a decimal (e.g. "answer": 5.0) or rename the test to reflect it only checks
plain integers; specifically modify the JSON string used when decoding
QuestionnaireResponseArgs (variable args) so that args.answer is validated for
the decimal-form input (and keep the same assertion branch that checks if case
.number(let value) to assert value == 5).
In `@Tests/AppTests/ValidationTests.swift`:
- Around line 94-105: The test currently uses a nonexistent linkId so it never
exercises maxLength; update stringExceedsMaxLength to validate against a real
string item with a maxLength constraint by creating or using an item/linkId
present in the engine (e.g., add a test QuestionnaireItem with linkId like
"testString" and maxLength = N) then call engine.validateAnswer(linkId:
"testString", answer: "a".repeating(N+1)) and assert the returned error
indicates exceedsMaxLength (rather than nil); reference
makeTestEngine(resourceName:), engine.validateAnswer(linkId:answer:), and the
stringExceedsMaxLength() test to locate and update the code.
---
Outside diff comments:
In `@Sources/App/Services/Call/CallSession.swift`:
- Around line 190-194: The code treats a nil from engine.nextQuestionString as
“no next question,” but nextQuestionString currently returns nil for both “no
next item” and “serialization/encoding failure,” which causes questionnaire
completion to be triggered on errors; update nextQuestionString to either throw
on serialization errors or return a discriminating enum/Result (e.g.,
.next(String) | .none | .error(Error)), then change this call in
CallSession.swift to await and handle the error case explicitly (catch the
thrown error or switch the Result/enum) — only call
handleQuestionnaireComplete(response:) when you have an explicit “no next”
result and propagate/log serialization errors instead of treating them as
completion; reference engine.nextQuestionString,
handleNextQuestionAvailable(nextQuestion:response:), and
handleQuestionnaireComplete(response:) when making the change.
---
Nitpick comments:
In `@Sources/App/Services/Questionnaire/FHIRQuestionnaireEngine`+EnableWhen.swift:
- Around line 110-123: The compareDecimalAnswer function currently converts
FHIRDecimal values to Double (via NSDecimalNumber.doubleValue) which can lose
precision; update it to compare using Decimal (or NSDecimalNumber.compare)
directly: extract responseDecimal.value?.decimal and expected.value?.decimal as
Decimal, then perform a decimal-native comparison (or add/overload
compareNumbers to accept Decimals) and use the resulting comparison to evaluate
the QuestionnaireItemOperator. Keep references to compareDecimalAnswer and
compareNumbers (or introduce compareNumbersDecimal/NSDecimalNumber.compare) so
reviewers can find the change.
In `@Sources/App/Services/Questionnaire/FHIRQuestionnaireEngine`+Navigation.swift:
- Around line 34-49: The closures that check editable/unanswered items are
duplicated (used in the enabledItems.first chains and in
unansweredEnabledItems); extract a helper like isEditableAndUnanswered(_ item:
QuestionnaireItem, answeredIds: Set<String>) -> Bool that performs the linkId
unwrap, readOnly check, and answeredIds containment test, then replace the
duplicated predicates in enabledItems.first and unansweredEnabledItems with
calls to this helper (for the required-first branch, combine the helper with the
required check: isEditableAndUnanswered(item, answeredIds) &&
(item.required?.value?.bool ?? false)).
In `@Tests/AppTests/EnableWhenTests.swift`:
- Around line 94-107: The test enableWhenCodingEqual relies on an implicit
mapping in the "q17" resource that maps the displayed answer "much-worse" to
coding "1"; update the test to make this dependency explicit by either adding a
short inline comment above the FHIRBuilder.enableWhen(...) and
engine.answerQuestion(...) calls explaining that "much-worse" maps to coding "1"
in resource "q17", or change the test to answer with the raw code value (e.g.,
use the coded value instead of "much-worse") so the equality check in
engine.evaluateCondition(condition) is self-documenting; reference the test
function enableWhenCodingEqual, the FHIRBuilder.enableWhen(...) call, and
engine.answerQuestion(linkId: "wellbeing-comparison", answer: ...) when making
the change.
- Around line 253-278: The test hierarchicalResponseStructure currently assumes
a fixed order in engine.hierarchicalResponse() by asserting topItems.count == 3
and checking specific indices; make it robust by locating items by linkId
instead of relying on array indices (e.g., find item where
item.linkId.value?.string == "blood-pressure-group" and assert its children
count, find items for "heart-rate" and "weight" and assert they have answers),
or if you prefer to keep the ordering assertions, add a concise comment
describing the expected vitalSigns structure so future editors know the
dependency; update the assertions in hierarchicalResponseStructure to use
search-by-linkId on topItems and validate counts/answers accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e8fefc2a-efe4-48a2-a75d-e9fb7826bf61
📒 Files selected for processing (20)
Sources/App/Resources/sessionConfig.jsonSources/App/Services/Call/CallSession+SectionTransition.swiftSources/App/Services/Call/CallSession.swiftSources/App/Services/Questionnaire/CallFlowCoordinator.swiftSources/App/Services/Questionnaire/EngageHF/EngageHFSections.swiftSources/App/Services/Questionnaire/FHIRQuestionnaireEngine+EnableWhen.swiftSources/App/Services/Questionnaire/FHIRQuestionnaireEngine+Navigation.swiftSources/App/Services/Questionnaire/FHIRQuestionnaireEngine+Simplification.swiftSources/App/Services/Questionnaire/FHIRQuestionnaireEngine.swiftSources/App/Services/Questionnaire/QuestionnaireResponseArgs.swiftSources/App/Services/Questionnaire/QuestionnaireResponseStore.swiftSources/App/Services/Questionnaire/SimplifiedQuestion.swiftSources/App/constants.swiftTests/AppTests/AppTests.swiftTests/AppTests/EnableWhenTests.swiftTests/AppTests/Helpers/QuestionnaireTestHelpers.swiftTests/AppTests/NavigationTests.swiftTests/AppTests/QuestionnaireResponseArgsTests.swiftTests/AppTests/SimplificationTests.swiftTests/AppTests/ValidationTests.swift
Comment on lines
+15
to
+24
| if let nextEngine = await coordinator.advanceToNextSection() { | ||
| let initialQuestion = await nextEngine.nextQuestionString(includeAllQuestions: true) | ||
| if let systemMessage = await coordinator.sectionSystemMessage( | ||
| for: nextEngine, initialQuestion: initialQuestion | ||
| ) { | ||
| try await handleNextSectionAvailable( | ||
| initialQuestion: initialQuestion, | ||
| systemMessage: systemMessage, | ||
| response: response | ||
| ) |
There was a problem hiding this comment.
Skip sections that have no answerable first question.
advanceToNextSection() only advances the index. If the next section has no askable item, initialQuestion is nil but this still updates the session and creates a response, leaving the model in a new section with nothing to ask.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/App/Services/Call/CallSession`+SectionTransition.swift around lines
15 - 24, advanceToNextSection() may land on sections with no answerable first
question because nextQuestionString(includeAllQuestions:) can return nil; update
the logic around calling advanceToNextSection(), nextQuestionString(...),
sectionSystemMessage(...), and handleNextSectionAvailable(...) to skip over any
advanced sections where initialQuestion is nil or sectionSystemMessage returns
nil: loop calling coordinator.advanceToNextSection() until it returns nil or you
obtain a non-nil initialQuestion and systemMessage, and only then call
handleNextSectionAvailable(...) and create/update the response; if
advanceToNextSection() returns nil, bail out without changing the
session/response. Ensure you reference the existing methods
coordinator.advanceToNextSection(), nextQuestionString(includeAllQuestions:),
coordinator.sectionSystemMessage(for:initialQuestion:), and
handleNextSectionAvailable(...) when making the change.
Comment on lines
+45
to
+49
| func handleAllSectionsComplete(response: OpenAIResponse) async throws { | ||
| let feedback = await coordinator.generateFeedback() | ||
| let systemMessage = Constants.feedback( | ||
| content: feedback ?? "Feedback failed to be retrieved." | ||
| ) |
There was a problem hiding this comment.
Pass nil through to Constants.feedback.
That helper already has a dedicated failure template for nil. Replacing nil with "Feedback failed to be retrieved." forces the success template and tells the model to read a fake feedback block and mention a symptom score even when feedback generation failed.
💡 Suggested fix
func handleAllSectionsComplete(response: OpenAIResponse) async throws {
let feedback = await coordinator.generateFeedback()
- let systemMessage = Constants.feedback(
- content: feedback ?? "Feedback failed to be retrieved."
- )
+ let systemMessage = Constants.feedback(content: feedback)
try await updateSession(systemMessage: systemMessage)
try await sendResponseCreate()
}📝 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
| func handleAllSectionsComplete(response: OpenAIResponse) async throws { | |
| let feedback = await coordinator.generateFeedback() | |
| let systemMessage = Constants.feedback( | |
| content: feedback ?? "Feedback failed to be retrieved." | |
| ) | |
| func handleAllSectionsComplete(response: OpenAIResponse) async throws { | |
| let feedback = await coordinator.generateFeedback() | |
| let systemMessage = Constants.feedback(content: feedback) | |
| try await updateSession(systemMessage: systemMessage) | |
| try await sendResponseCreate() | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/App/Services/Call/CallSession`+SectionTransition.swift around lines
45 - 49, In handleAllSectionsComplete, stop substituting a placeholder string
when feedback generation fails; call Constants.feedback(content: feedback) with
feedback (which may be nil) so the helper can use its built-in failure
template—update the systemMessage construction in handleAllSectionsComplete to
pass the optional returned from coordinator.generateFeedback() directly to
Constants.feedback.
Comment on lines
+54
to
+59
| func handleProcessingError(error: any Error, response: OpenAIResponse) async throws { | ||
| logger.error("Error processing questionnaire: \(error)") | ||
| try await sendFunctionOutput( | ||
| callId: response.callId ?? "", | ||
| output: "Failed to process questionnaire" | ||
| ) |
There was a problem hiding this comment.
Finish the error path with response.create.
Every other function-call output path here follows the tool output with sendResponseCreate(). Without it, the model may never consume the failure message and the call can stall after an error.
💡 Suggested fix
func handleProcessingError(error: any Error, response: OpenAIResponse) async throws {
logger.error("Error processing questionnaire: \(error)")
try await sendFunctionOutput(
callId: response.callId ?? "",
output: "Failed to process questionnaire"
)
+ try await sendResponseCreate()
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/App/Services/Call/CallSession`+SectionTransition.swift around lines
54 - 59, The error path in handleProcessingError currently logs and calls
sendFunctionOutput but never sends the final response.create message; after
calling sendFunctionOutput (using the same callId from response.callId ?? ""),
also invoke sendResponseCreate(...) with the failure output so the model
receives the tool result and the call completes; ensure you use the same
OpenAIResponse/response context and mirror other code paths that call
sendResponseCreate after sendFunctionOutput.
Comment on lines
254
to
257
| /// Persist the current response to disk. | ||
| func save() { | ||
| store.saveResponse(response) | ||
| store.saveResponse(hierarchicalResponse()) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how loadResponse handles the response structure
rg -n "loadResponse|saveResponse" --type swift -A 10Repository: StanfordBDHG/ENGAGE-HF-AI-Voice
Length of output: 6755
🏁 Script executed:
#!/bin/bash
# Find QuestionnaireResponse definition and hierarchicalResponse implementation
rg -n "struct QuestionnaireResponse|class QuestionnaireResponse|func hierarchicalResponse" --type swift -A 15Repository: StanfordBDHG/ENGAGE-HF-AI-Voice
Length of output: 7980
🏁 Script executed:
#!/bin/bash
# Search for how saved responses are consumed/read
rg -n "loadResponse\(\)|currentResponse\(\)" --type swift -B 2 -A 5Repository: StanfordBDHG/ENGAGE-HF-AI-Voice
Length of output: 7140
🏁 Script executed:
#!/bin/bash
# Look for any response format handling or compatibility checks
rg -n "item|Item|hierarchical|flat" Sources/App/Services/Questionnaire/ --type swift -B 2 -A 2 | head -100Repository: StanfordBDHG/ENGAGE-HF-AI-Voice
Length of output: 10016
🏁 Script executed:
#!/bin/bash
# Search for any tests that save, load, and answer questions
rg -n "answerQuestion.*loadResponse|loadResponse.*answerQuestion|save.*answerQuestion" --type swift -B 5 -A 10Repository: StanfordBDHG/ENGAGE-HF-AI-Voice
Length of output: 57
🏁 Script executed:
#!/bin/bash
# Check if there's any flattening logic when loading responses
rg -n "flattenResponseItems|flattenItems" --type swift -B 2 -A 5Repository: StanfordBDHG/ENGAGE-HF-AI-Voice
Length of output: 12872
🏁 Script executed:
#!/bin/bash
# Look at QuestionnaireResponse structure - check if item nesting is expected
fd -e swift -path "*/Sources/*" -type f | xargs grep -l "QuestionnaireResponse" | head -5 | xargs grep -n "class QuestionnaireResponse\|struct QuestionnaireResponse" -A 3Repository: StanfordBDHG/ENGAGE-HF-AI-Voice
Length of output: 245
🏁 Script executed:
#!/bin/bash
# Find Package.swift to see dependencies
cat Package.swift | grep -A 20 -B 5 "QuestionnaireResponse\|FHIR"Repository: StanfordBDHG/ENGAGE-HF-AI-Voice
Length of output: 1875
🏁 Script executed:
#!/bin/bash
# Check the full init method and loadResponse handling
rg -n "init\(" Sources/App/Services/Questionnaire/FHIRQuestionnaireEngine.swift -A 20 | head -60Repository: StanfordBDHG/ENGAGE-HF-AI-Voice
Length of output: 850
🏁 Script executed:
#!/bin/bash
# Look for any response flattening or normalization after loading
rg -n "store.loadResponse\(\)|flattenResponseItems" Sources/App/Services/Questionnaire/FHIRQuestionnaireEngine.swift -B 3 -A 10Repository: StanfordBDHG/ENGAGE-HF-AI-Voice
Length of output: 1286
🏁 Script executed:
#!/bin/bash
# Search for test patterns combining save, load, and answer
rg -n "save\(\)|answerQuestion|loadResponse" Tests/AppTests/ --type swift -B 2 -A 2 | grep -E "(save|answerQuestion|loadResponse)" | head -40Repository: StanfordBDHG/ENGAGE-HF-AI-Voice
Length of output: 4483
🏁 Script executed:
#!/bin/bash
# Check if loadResponse has any post-processing or flattening
sed -n '60,78p' Sources/App/Services/Questionnaire/QuestionnaireResponseStore.swiftRepository: StanfordBDHG/ENGAGE-HF-AI-Voice
Length of output: 848
🏁 Script executed:
#!/antml/bash
# Look at the full init to confirm no flattening after loadResponse
sed -n '88,119p' Sources/App/Services/Questionnaire/FHIRQuestionnaireEngine.swiftRepository: StanfordBDHG/ENGAGE-HF-AI-Voice
Length of output: 1096
This change introduces a critical incompatibility in the response persistence flow.
Saving hierarchicalResponse() breaks the system because:
loadResponse()deserializes the hierarchical structure without flattening it, leavingresponse.itemwith nested itemsanswerQuestion()appends items toresponse.itemexpecting a flat structure, corrupting the hierarchy and breaking answer storageupdateFinishedState()and score calculations rely on a flatresponse.itemstructure
The code has no mechanism to flatten responses after loading (despite flattenResponseItems() existing as unused utility). Tests only validate single-session workflows and do not cover the critical reload-after-save scenario.
Before saving hierarchical format, ensure either:
- Load path flattens responses after deserialization, or
- Answer operations handle both flat and hierarchical structures, or
- Persist the flat response and generate hierarchical on-demand via
hierarchicalResponse()
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/App/Services/Questionnaire/FHIRQuestionnaireEngine.swift` around
lines 254 - 257, The save() call currently persists hierarchicalResponse(),
which breaks the flat-item assumptions in loadResponse(), answerQuestion(),
updateFinishedState(), and scoring; change persistence to store the flat
response and generate hierarchy on-demand: modify save() to persist the
flattened response (use the existing flattenResponseItems() utility or a
flatResponse() helper) and keep hierarchicalResponse() as a transient generator,
or alternatively ensure loadResponse() calls flattenResponseItems() immediately
after deserialization so response.item remains flat for answerQuestion(),
updateFinishedState(), and scoring; update references to hierarchicalResponse(),
loadResponse(), and flattenResponseItems() accordingly.
Comment on lines
+69
to
+71
| default: | ||
| return true | ||
| } |
There was a problem hiding this comment.
Returning true for unsupported answer types may cause unintended enablement.
The default case returns true, which means items with unsupported enableWhen answer types (like date, time, quantity, reference, attachment) will always be treated as enabled. Consider returning false or logging a warning to surface unsupported configurations.
🛡️ Suggested fix
case .string(let string):
return compareStringAnswer(responseAnswer, expected: string, comparison: comparison)
default:
- return true
+ return false // Unsupported answer type - treat as condition not met
}📝 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
| default: | |
| return true | |
| } | |
| default: | |
| return false // Unsupported answer type - treat as condition not met | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/App/Services/Questionnaire/FHIRQuestionnaireEngine`+EnableWhen.swift
around lines 69 - 71, The switch's default currently returns true in
FHIRQuestionnaireEngine+EnableWhen (the enableWhen evaluation routine) causing
unsupported answer types to be treated as enabled; change the default branch to
return false and emit a warning/log (use the engine's logger) that includes the
unsupported answer type/value (e.g., the answer.type or answerValue variable) so
misconfigured enableWhen rules are visible; update any unit tests for the
enableWhen evaluator (e.g., tests referencing evaluateEnableWhen/isEnabled) to
expect false for unsupported types.
Comment on lines
+92
to
+108
| for ext in extensions { | ||
| let url = ext.url.value?.url.absoluteString ?? "" | ||
| if url == Self.minValueURL { | ||
| if case .integer(let val) = ext.value { | ||
| minValue = Double(val.value?.integer ?? 0) | ||
| } else if case .decimal(let val) = ext.value { | ||
| minValue = NSDecimalNumber(decimal: val.value?.decimal ?? 0).doubleValue | ||
| } | ||
| } else if url == Self.maxValueURL { | ||
| if case .integer(let val) = ext.value { | ||
| maxValue = Double(val.value?.integer ?? 0) | ||
| } else if case .decimal(let val) = ext.value { | ||
| maxValue = NSDecimalNumber(decimal: val.value?.decimal ?? 0).doubleValue | ||
| } | ||
| } | ||
| } | ||
| return (minValue, maxValue) |
There was a problem hiding this comment.
Avoid turning missing min/max primitives into 0.
val.value?.integer ?? 0 and val.value?.decimal ?? 0 make an empty minValue/maxValue extension behave like a real bound of zero. That can reject valid answers and diverges from the existing minValue(item:)/maxValue(item:) helpers, which ignore missing primitive values.
💡 Suggested fix
for ext in extensions {
let url = ext.url.value?.url.absoluteString ?? ""
if url == Self.minValueURL {
if case .integer(let val) = ext.value {
- minValue = Double(val.value?.integer ?? 0)
+ minValue = val.value.map { Double($0.integer) }
} else if case .decimal(let val) = ext.value {
- minValue = NSDecimalNumber(decimal: val.value?.decimal ?? 0).doubleValue
+ if let decimal = val.value?.decimal {
+ minValue = NSDecimalNumber(decimal: decimal).doubleValue
+ }
}
} else if url == Self.maxValueURL {
if case .integer(let val) = ext.value {
- maxValue = Double(val.value?.integer ?? 0)
+ maxValue = val.value.map { Double($0.integer) }
} else if case .decimal(let val) = ext.value {
- maxValue = NSDecimalNumber(decimal: val.value?.decimal ?? 0).doubleValue
+ if let decimal = val.value?.decimal {
+ maxValue = NSDecimalNumber(decimal: decimal).doubleValue
+ }
}
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@Sources/App/Services/Questionnaire/FHIRQuestionnaireEngine`+Simplification.swift
around lines 92 - 108, The loop that reads extensions in
FHIRQuestionnaireEngine+Simplification.swift is defaulting missing primitive
values to zero (using val.value?.integer ?? 0 and val.value?.decimal ?? 0),
which incorrectly treats absent min/max as a real bound; update the handling
inside the cases for .integer and .decimal so you only assign to
minValue/maxValue when val.value?.integer or val.value?.decimal is non-nil
(e.g., unwrap the optional and convert to Double/NSDecimalNumber only when
present), leaving minValue/maxValue as nil when the primitive is missing; keep
the checks against Self.minValueURL and Self.maxValueURL and the existing
ext.value pattern matching.
Comment on lines
+132
to
+145
| if let numericValue = (answer as? Int).map({ Double($0) }) ?? (answer as? Double) { | ||
| if let min = simplified.minValue, numericValue < min { | ||
| return .belowMinimum(min) | ||
| } | ||
| if let max = simplified.maxValue, numericValue > max { | ||
| return .aboveMaximum(max) | ||
| } | ||
| } | ||
|
|
||
| if let stringValue = answer as? String, let maxLen = simplified.maxLength { | ||
| if stringValue.count > maxLen { | ||
| return .exceedsMaxLength(maxLen) | ||
| } | ||
| } |
There was a problem hiding this comment.
Numeric range validation is bypassed for string answers.
This only checks Int and Double, but the new tests already pass "65" as a String to numeric question heart-rate. Inputs like "29" or "300" will skip min/max validation here if they arrive as text.
💡 Suggested fix
- if let numericValue = (answer as? Int).map({ Double($0) }) ?? (answer as? Double) {
+ if let numericValue =
+ (answer as? Int).map(Double.init)
+ ?? (answer as? Double)
+ ?? (answer as? String).flatMap(Double.init)
+ {
if let min = simplified.minValue, numericValue < min {
return .belowMinimum(min)
}
if let max = simplified.maxValue, numericValue > max {
return .aboveMaximum(max)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@Sources/App/Services/Questionnaire/FHIRQuestionnaireEngine`+Simplification.swift
around lines 132 - 145, The numeric-range check in
FHIRQuestionnaireEngine+Simplification.swift currently only handles Int/Double
and thus skips numeric validation when answers are numeric strings; update the
validation around the existing numeric block that uses
simplified.minValue/simplified.maxValue and answer so that if answer is a String
you attempt to parse it to a Double (e.g. via Double(stringValue)) and then
apply the same belowMinimum/aboveMaximum checks, while preserving the separate
string maxLength check (exceedsMaxLength) for non-numeric strings; reference
simplified.minValue, simplified.maxValue, answer, and the
.belowMinimum/.aboveMaximum/.exceedsMaxLength result cases when making the
change.
| let jsonData = try encoder.encode(response) | ||
| let dataToWrite = try encryptIfNeeded(jsonData, logger: logger) | ||
| try dataToWrite.write(to: fileURL(phoneNumber), options: .atomic) | ||
| try dataToWrite.write(to: fileURL(phoneNumber)) |
There was a problem hiding this comment.
Consider restoring atomic write for data integrity.
The removal of the .atomic option means the file write is no longer atomic. If the process crashes or is interrupted mid-write, this could leave a corrupted partial file. For encrypted patient health data, atomic writes provide a safety guarantee: the file is either fully written or left unchanged.
🛡️ Proposed fix to restore atomic write
- try dataToWrite.write(to: fileURL(phoneNumber))
+ try dataToWrite.write(to: fileURL(phoneNumber), options: .atomic)📝 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
| try dataToWrite.write(to: fileURL(phoneNumber)) | |
| try dataToWrite.write(to: fileURL(phoneNumber), options: .atomic) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/App/Services/Questionnaire/QuestionnaireResponseStore.swift` at line
98, The write removed the .atomic option so writes in QuestionnaireResponseStore
(the call that does dataToWrite.write(to: fileURL(phoneNumber))) are no longer
atomic; restore atomicity by using the Data write overload that accepts
FileHandle/NSDataWritingOptions and pass the .atomic option (e.g., use
dataToWrite.write(to: fileURL(phoneNumber), options: .atomic)) in the method
that persists responses in QuestionnaireResponseStore so partial/corrupt files
cannot be left behind.
Comment on lines
+42
to
+51
| @Test("Decode integer-looking decimal (e.g. 5.0) as integer") | ||
| func decodeIntegerLookingDecimal() throws { | ||
| // JSON `5` without decimal point should decode as integer | ||
| let json = #"{"linkId": "q1", "answer": 5}"# | ||
| let args = try JSONDecoder().decode(QuestionnaireResponseArgs.self, from: Data(json.utf8)) | ||
| if case .number(let value) = args.answer { | ||
| #expect(value == 5) | ||
| } else { | ||
| Issue.record("Expected .number(5), got \(String(describing: args.answer))") | ||
| } |
There was a problem hiding this comment.
Rename this test or exercise 5.0 for real.
Line 45 uses 5, so this only proves the plain integer branch. The integer-looking-decimal path named in the test would still be untested.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Tests/AppTests/QuestionnaireResponseArgsTests.swift` around lines 42 - 51,
The test decodeIntegerLookingDecimal currently uses JSON with integer 5, so it
doesn't exercise the decimal-to-integer decoding path; update the test in
function decodeIntegerLookingDecimal to use a JSON value with a decimal (e.g.
"answer": 5.0) or rename the test to reflect it only checks plain integers;
specifically modify the JSON string used when decoding QuestionnaireResponseArgs
(variable args) so that args.answer is validated for the decimal-form input (and
keep the same assertion branch that checks if case .number(let value) to assert
value == 5).
Comment on lines
+94
to
+105
| @Test("String answer exceeding maxLength is rejected") | ||
| @MainActor | ||
| func stringExceedsMaxLength() async throws { | ||
| try await withTestApp { app in | ||
| let engine = try makeTestEngine(resourceName: "vitalSigns", app: app) | ||
|
|
||
| // We validate a builder-made item with maxLength through validateAnswer | ||
| // Since vitalSigns doesn't have string items, we test the validateAnswer | ||
| // directly with an item that doesn't exist — should return nil (no item found) | ||
| let error = engine.validateAnswer(linkId: "nonexistent", answer: "long text") | ||
| #expect(error == nil) | ||
| } |
There was a problem hiding this comment.
This test never exercises maxLength.
Line 103 uses a nonexistent linkId and asserts nil, so it only covers the "item not found" path. A broken exceedsMaxLength implementation would still pass unnoticed.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Tests/AppTests/ValidationTests.swift` around lines 94 - 105, The test
currently uses a nonexistent linkId so it never exercises maxLength; update
stringExceedsMaxLength to validate against a real string item with a maxLength
constraint by creating or using an item/linkId present in the engine (e.g., add
a test QuestionnaireItem with linkId like "testString" and maxLength = N) then
call engine.validateAnswer(linkId: "testString", answer: "a".repeating(N+1)) and
assert the returned error indicates exceedsMaxLength (rather than nil);
reference makeTestEngine(resourceName:), engine.validateAnswer(linkId:answer:),
and the stringExceedsMaxLength() test to locate and update the code.
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
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.
More FHIR capabilities
♻️ Current situation & Problem
This pull request contains primarily additional features that may be useful for other projects, but not needed here directly - it does offer more compatibility to FHIR though, which might be super helpful for an extracted package to be used by other voice agent implementations.
⚙️ Release Notes
initialandreadonlyvaluesrepeatssupportCode of Conduct & Contributing Guidelines
By creating and submitting this pull request, you agree to follow our Code of Conduct and Contributing Guidelines:
Summary by CodeRabbit
New Features
Bug Fixes
Tests