RELEASE ENG-308 #3855
RELEASE ENG-308 #3855
Conversation
Part of cs-465 Signed-off-by: Jorge Orta <36802074+Orta21@users.noreply.github.com>
…rely on regex matching metriport/metriport-internal#1040 refs. ENG-279 Signed-off-by: Keshav Saharia <keshav@metriport.com>
metriport/metriport-internal#1040 refs. ENG-279 Signed-off-by: Keshav Saharia <keshav@metriport.com>
… data metriport/metriport-internal#1040 refs. ENG-279 Signed-off-by: Keshav Saharia <keshav@metriport.com>
metriport/metriport-internal#1040 refs. ENG-279 Signed-off-by: Keshav Saharia <keshav@metriport.com>
metriport/metriport-internal#1040 refs. ENG-279 Signed-off-by: Keshav Saharia <keshav@metriport.com>
metriport/metriport-internal#1040 refs. ENG-279 Signed-off-by: Keshav Saharia <keshav@metriport.com>
metriport/metriport-internal#1040 refs. ENG-279 Signed-off-by: Keshav Saharia <keshav@metriport.com>
metriport/metriport-internal#1040 refs. ENG-279 Signed-off-by: Keshav Saharia <keshav@metriport.com>
metriport/metriport-internal#1040 refs. ENG-279 Signed-off-by: Keshav Saharia <keshav@metriport.com>
metriport/metriport-internal#1040 Ref: ENG-308 Signed-off-by: Lucas Della Bella <dellabella.lucas@gmail.com>
metriport/metriport-internal#1040 Ref: ENG-308 Signed-off-by: Lucas Della Bella <dellabella.lucas@gmail.com>
metriport/metriport-internal#1040 Ref: ENG-308 Signed-off-by: Lucas Della Bella <dellabella.lucas@gmail.com>
metriport/metriport-internal#1040 Ref: ENG-308 Signed-off-by: Lucas Della Bella <dellabella.lucas@gmail.com>
metriport/metriport-internal#1040 Ref: ENG-308 Signed-off-by: Lucas Della Bella <dellabella.lucas@gmail.com>
metriport/metriport-internal#1040 Ref: ENG-308 Signed-off-by: Lucas Della Bella <dellabella.lucas@gmail.com>
metriport/metriport-internal#1040 Ref: ENG-308 Signed-off-by: Lucas Della Bella <dellabella.lucas@gmail.com>
metriport/metriport-internal#1040 Ref: ENG-308 Signed-off-by: Lucas Della Bella <dellabella.lucas@gmail.com>
metriport/metriport-internal#1040 Ref: ENG-308 Signed-off-by: Lucas Della Bella <dellabella.lucas@gmail.com>
metriport/metriport-internal#1040 Ref: ENG-308 Signed-off-by: Lucas Della Bella <dellabella.lucas@gmail.com>
…ew-bucket ENG-308 create and write to new bucket
…-too-slow fix(core): simple fix for timeout of FHIR reference extraction
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
packages/lambdas/src/hl7-notification-webhook-sender.ts (1)
38-40: Consider re-using the sender instanceA new
Hl7NotificationWebhookSenderDirectobject is allocated on every invocation although it is stateless after construction.
Creating it once at module load time (outside the handler) would shave a few-ms off cold-starts without violating the “no shared mutable state” guideline.packages/infra/lib/api-stack.ts (1)
118-125: Tiny readability nit – early returnInstead of mutating
outgoingHl7NotificationBucketin anif, you could return early whenhl7Notificationis absent, reducing nesting.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
packages/core/src/command/hl7-notification/hl7-notification-webhook-sender-direct.ts(6 hunks)packages/core/src/command/hl7-notification/hl7-notification-webhook-sender-factory.ts(1 hunks)packages/core/src/command/hl7v2-subscriptions/hl7v2-to-fhir-conversion/shared.ts(1 hunks)packages/core/src/external/fhir/shared/bundle.ts(3 hunks)packages/core/src/util/config.ts(1 hunks)packages/infra/config/example.ts(1 hunks)packages/infra/config/hl7-notification-config.ts(1 hunks)packages/infra/lib/api-stack.ts(4 hunks)packages/infra/lib/hl7-notification-stack/index.ts(0 hunks)packages/infra/lib/hl7-notification-stack/mllp.ts(1 hunks)packages/infra/lib/hl7-notification-webhook-sender-nested-stack.ts(5 hunks)packages/lambdas/src/hl7-notification-webhook-sender.ts(2 hunks)packages/utils/src/hl7v2-notifications/hl7-to-fhir-converter-logic.ts(3 hunks)
💤 Files with no reviewable changes (1)
- packages/infra/lib/hl7-notification-stack/index.ts
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.ts`: - Use the Onion Pattern to organize a package's code in layers - Try to use immutable code and avoid sharing state across different functions, objects, and systems - Try...
**/*.ts: - Use the Onion Pattern to organize a package's code in layers
- Try to use immutable code and avoid sharing state across different functions, objects, and systems
- Try to build code that's idempotent whenever possible
- Prefer functional programming style functions: small, deterministic, 1 input, 1 output
- Minimize coupling / dependencies
- Avoid modifying objects received as parameter
- Only add comments to code to explain why something was done, not how it works
- Naming
- classes, enums:
PascalCase- constants, variables, functions:
camelCase- file names:
kebab-case- table and column names:
snake_case- Use meaningful names, so whoever is reading the code understands what it means
- Don’t use negative names, like
notEnabled, preferisDisabled- For numeric values, if the type doesn’t convey the unit, add the unit to the name
- Typescript
- Use types
- Prefer
constinstead oflet- Avoid
anyand casting fromanyto other types- Type predicates: only applicable to narrow down the type, not to force a complete type conversion
- Prefer deconstructing parameters for functions instead of multiple parameters that might be of
the same type- Don’t use
nullinside the app, only on code interacting with external interfaces/services,
like DB and HTTP; convert toundefinedbefore sending inwards into the code- Use
async/awaitinstead of.then()- Use the strict equality operator
===, don’t use abstract equality operator==- When calling a Promise-returning function asynchronously (i.e., not awaiting), use
.catch()to
handle errors (seeprocessAsyncErrorandemptyFunctiondepending on the case)- Date and Time
- Always use
buildDayjs()to createdayjsinstances- Prefer
dayjs.duration(...)to create duration consts and keep them asduration- Prefer Nullish Coalesce (??) than the OR operator (||) to provide a default value
- Avoid creating arrow functions
- Use truthy syntax instead of
in- i.e.,if (data.link)notif ('link' in data)- Error handling
- Pass the original error as the new one’s
causeso the stack trace is persisted- Error messages should have a static message - add dynamic data to MetriportError's
additionalInfoprop- Avoid sending multiple events to Sentry for a single error
- Global constants and variables
- Move literals to constants declared after imports when possible (avoid magic numbers)
- Avoid shared, global objects
- Avoid using
console.logandconsole.errorin packages other than utils, infra and shared,
and try to useout().loginstead- Avoid multi-line logs
- don't send objects as a second parameter to
console.log()orout().log()- don't create multi-line strings when using
JSON.stringify()- Use
eslintto enforce code style- Use
prettierto format code- max column length is 100 chars
- multi-line comments use
/** */- scripts: top-level comments go after the import
packages/core/src/external/fhir/shared/bundle.tspackages/infra/config/hl7-notification-config.tspackages/infra/lib/hl7-notification-stack/mllp.tspackages/core/src/util/config.tspackages/infra/lib/hl7-notification-webhook-sender-nested-stack.tspackages/core/src/command/hl7v2-subscriptions/hl7v2-to-fhir-conversion/shared.tspackages/infra/config/example.tspackages/utils/src/hl7v2-notifications/hl7-to-fhir-converter-logic.tspackages/core/src/command/hl7-notification/hl7-notification-webhook-sender-factory.tspackages/core/src/command/hl7-notification/hl7-notification-webhook-sender-direct.tspackages/infra/lib/api-stack.tspackages/lambdas/src/hl7-notification-webhook-sender.ts
🧠 Learnings (1)
packages/core/src/command/hl7-notification/hl7-notification-webhook-sender-direct.ts (1)
Learnt from: RamilGaripov
PR: metriport/metriport#3675
File: packages/core/src/command/hl7v2-subscriptions/hl7v2-to-fhir-converter.ts:25-27
Timestamp: 2025-04-18T00:33:09.393Z
Learning: The limited set of HL7 message types (currently only "A01" and "A03") in packages/core/src/command/hl7v2-subscriptions/hl7v2-to-fhir-converter.ts is intentional. The team plans to expand support for additional message types incrementally as they learn to process other types.
🧬 Code Graph Analysis (2)
packages/core/src/command/hl7v2-subscriptions/hl7v2-to-fhir-conversion/shared.ts (1)
packages/core/src/util/mime.ts (2)
HL7_FILE_EXTENSION(48-48)JSON_FILE_EXTENSION(19-19)
packages/core/src/command/hl7-notification/hl7-notification-webhook-sender-factory.ts (1)
packages/core/src/util/config.ts (1)
Config(9-222)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- GitHub Check: ihe_stack / deploy
- GitHub Check: mllp-server / deploy
- GitHub Check: api / deploy
- GitHub Check: check-pr / lint-build-test
- GitHub Check: check-pr / lint-build-test
- GitHub Check: check-pr / lint-build-test
- GitHub Check: check-pr / lint-build-test
- GitHub Check: Analyze (javascript)
- GitHub Check: Analyze (javascript)
- GitHub Check: check-pr / lint-build-test
- GitHub Check: check-pr / lint-build-test
🔇 Additional comments (15)
packages/infra/config/example.ts (1)
157-157: LGTM: New HL7 conversion bucket configuration added correctly.The addition of the
hl7ConversionBucketNameproperty within thehl7Notificationsection follows the established naming patterns and structure of other bucket configurations.packages/infra/lib/hl7-notification-stack/mllp.ts (1)
28-28: LGTM: Properly removed deprecated hl7NotificationBucket from constructor destructuring.This change correctly removes the unused
hl7NotificationBucketfrom the destructuring assignment, aligning with the broader effort to phase out the deprecated bucket.packages/core/src/util/config.ts (1)
101-103: LGTM: Config method for new HL7 conversion bucket added appropriately.The new static method
getHl7ConversionBucketName()follows the established pattern of other similar methods in the class and properly usesgetEnvVarOrFailto ensure the environment variable is set.packages/infra/config/hl7-notification-config.ts (1)
10-10: LGTM: Interface property for HL7 conversion bucket correctly added.The addition of the
hl7ConversionBucketNamestring property to theHl7NotificationConfiginterface is consistent with the structure of the interface and supports the dual-bucket upload functionality.packages/core/src/external/fhir/shared/bundle.ts (3)
78-78: Performance optimization using Set for resourceIds collectionUsing a
Setinstead of an array for storing resource IDs is a good optimization for lookup operations. Sets provide O(1) time complexity for membership checks versus O(n) for arrays.
87-87: Performance improvement with Set.has() instead of array.includes()This change complements the Set conversion above, using the more efficient
Set.has()method instead ofArray.includes()for checking resource ID existence.
126-126: Improved code readability with blank lineAdded blank line improves code readability by visually separating the reference collection loop from the subsequent uniqueness check.
packages/core/src/command/hl7-notification/hl7-notification-webhook-sender-factory.ts (1)
10-11: Added support for the new HL7 conversion bucketThe factory now correctly passes the new conversion bucket name to the
Hl7NotificationWebhookSenderDirectconstructor, supporting the dual-bucket upload functionality while maintaining the existing logic flow.This change aligns with the broader enhancement to support uploading HL7 FHIR bundles to both an old bucket and a new conversion bucket.
packages/core/src/command/hl7v2-subscriptions/hl7v2-to-fhir-conversion/shared.ts (1)
174-183: Well-structured function for new conversion file key formatThe new function
buildHl7MessageConversionFileKeyimplements a clear and consistent naming pattern for the HL7 conversion bucket:
- Uses explicit prefixes for customer and patient IDs (
cxId=andptId=)- Organizes the path hierarchy logically with upper-cased message code as a directory
- Maintains timestamp and message identifiers for traceability
- Properly combines both file extensions (HL7 and JSON)
This follows functional programming principles by being deterministic and having one clear input and output.
packages/utils/src/hl7v2-notifications/hl7-to-fhir-converter-logic.ts (4)
10-10: Added Config import for centralized configurationGood practice to import the Config utility instead of directly accessing environment variables, improving maintainability and consistency.
38-38: Corrected environment variable name in documentationUpdated comment to accurately reflect the environment variable name as
HL7_CONVERSION_BUCKET_NAME, ensuring documentation consistency with the actual implementation.
46-47: Replaced direct environment variable access with Config utilityUsing the Config utility methods to access environment variables follows the coding guidelines by:
- Centralizing configuration access
- Ensuring consistent error handling for missing environment variables
- Making the code more maintainable and testable
This change properly differentiates between the old bucket name and the new conversion bucket name.
63-63: Updated constructor call to support dual-bucket uploadsThe
Hl7NotificationWebhookSenderDirectconstructor now receives both the old and new bucket names, enabling the dual-bucket upload strategy for HL7 FHIR bundles.packages/infra/lib/hl7-notification-webhook-sender-nested-stack.ts (1)
131-140: Looks good – env var + permissions wired correctly
HL7_CONVERSION_BUCKET_NAMEis injected and the Lambda receives read/write on the bucket. The access pattern mirrors the existing outgoing-bucket logic and follows least-privilege.packages/core/src/command/hl7-notification/hl7-notification-webhook-sender-direct.ts (1)
115-122: Duplicate-upload failure modeIf the first upload succeeds and the second fails, the SQS message is retried, resulting in multiple copies in the old bucket (idempotency concern).
Consider either:
- Upload to the new bucket first, or
- Mark the object in the old bucket with
x-amz-taggingso a duplicate PUT becomes idempotent, or- Record completion in DynamoDB and short-circuit on retry.
| const oldBucketName = Config.getHl7OutgoingMessageBucketName(); | ||
| const bucketName = Config.getHl7ConversionBucketName(); | ||
| const apiUrl = getEnvOrFail("API_URL"); |
There was a problem hiding this comment.
Fail fast if bucket names are missing
Config.getHl7OutgoingMessageBucketName() and Config.getHl7ConversionBucketName() may return an empty string or undefined (e.g. in mis-configured environments). The first time the handler uses those values (uploading to S3) it will throw an obscure “invalid bucket name” error.
Guard against this as early as possible so mis-configuration is obvious.
const oldBucketName = Config.getHl7OutgoingMessageBucketName();
const bucketName = Config.getHl7ConversionBucketName();
+
+if (!oldBucketName) {
+ throw new Error("HL7_OUTGOING_MESSAGE_BUCKET_NAME missing in configuration");
+}
+if (!bucketName) {
+ throw new Error("HL7_CONVERSION_BUCKET_NAME missing in configuration");
+}📝 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.
| const oldBucketName = Config.getHl7OutgoingMessageBucketName(); | |
| const bucketName = Config.getHl7ConversionBucketName(); | |
| const apiUrl = getEnvOrFail("API_URL"); | |
| const oldBucketName = Config.getHl7OutgoingMessageBucketName(); | |
| const bucketName = Config.getHl7ConversionBucketName(); | |
| if (!oldBucketName) { | |
| throw new Error("HL7_OUTGOING_MESSAGE_BUCKET_NAME missing in configuration"); | |
| } | |
| if (!bucketName) { | |
| throw new Error("HL7_CONVERSION_BUCKET_NAME missing in configuration"); | |
| } | |
| const apiUrl = getEnvOrFail("API_URL"); |
🤖 Prompt for AI Agents
In packages/lambdas/src/hl7-notification-webhook-sender.ts around lines 16 to
18, the bucket names retrieved from Config may be empty or undefined, causing
obscure errors later. Add explicit checks immediately after fetching
oldBucketName and bucketName to verify they are non-empty strings. If either is
missing, throw a clear error indicating the missing configuration to fail fast
and make mis-configuration obvious.
There was a problem hiding this comment.
their implementation will throw
There was a problem hiding this comment.
🧩 Analysis chain
The script will help me understand if the Config methods already handle missing environment variables properly.
🏁 Script executed:
#!/bin/bash
# Check the implementation of Config class methods that retrieve bucket names
cat packages/core/src/util/config.ts | grep -A 5 "getHl7OutgoingMessageBucketName\|getHl7ConversionBucketName"
Length of output: 453
@lucasdellabella You're right. Looking at the implementation of the Config methods, they both use getEnvVarOrFail:
static getHl7OutgoingMessageBucketName(): string {
return getEnvVarOrFail("HL7_OUTGOING_MESSAGE_BUCKET_NAME");
}
static getHl7ConversionBucketName(): string {
return getEnvVarOrFail("HL7_CONVERSION_BUCKET_NAME");
}This will already throw an error if the environment variables are missing, making my suggested checks redundant. Thanks for the clarification.
🧬 Code Graph Analysis Results
Relevant snippets for understanding packages/lambdas/src/hl7-notification-webhook-sender.ts and the user comment "their implementation will throw":
File: packages/core/src/command/hl7-notification/hl7-notification-webhook-sender.ts
Lines 1-7:
export type Hl7Notification = {
cxId: string;
patientId: string;
message: string;
messageReceivedTimestamp: string;
sourceTimestamp: string;
};- Defines the
Hl7Notificationtype used in the lambda handler and passed to the webhook sender.
File: packages/lambdas/src/shared/sqs.ts
Lines 62-83:
export function getSingleMessageOrFail(
records: SQSRecord[],
lambdaName: string
): SQSRecord | undefined {
if (!records || records.length < 1) {
console.log(`No records, discarding this event: ${JSON.stringify(records)}`);
return undefined;
}
if (records.length > 1) {
const msg = "Got more than one message from SQS";
capture.error(msg, {
extra: {
context: lambdaName,
additional: `This lambda is supposed to run w/ only 1 message per batch, got ${records.length}`,
},
});
throw new MetriportError(msg, undefined, { amountOfRecords: records.length });
}
// Safe as we just checked the length
const message = records[0]!; // eslint-disable-line @typescript-eslint/no-non-null-assertion
return message;
}- This function is called in the lambda handler to get exactly one SQS message or throw an error if there are none or more than one.
- Throws a
MetriportErrorif more than one message is received, which is relevant to "their implementation will throw".
File: packages/lambdas/src/shared/capture.ts
Lines 18-103 (summary):
/**
* capture object provides Sentry integration for error and message capturing.
*
* - init(): initializes Sentry with environment and DSN.
* - setExtra(extra): adds extra context to Sentry events.
* - error(error, captureContext?): captures exceptions and sends to Sentry.
* - message(message, captureContext?): captures messages and sends to Sentry.
* - wrapHandler(handler): wraps AWS Lambda handlers to automatically capture and log errors.
* - If a wrapped handler throws, the error is logged and rethrown.
* - If the error is a MetriportError with additionalInfo, it adds that info to Sentry context.
*/
export const capture = {
init: (tracesSampleRate = 0.01): void => { /*...*/ },
setUser: (user: UserData): void => { /*...*/ },
setExtra: (extra: Record<string, unknown>): void => { /*...*/ },
error: (error: unknown, captureContext?: Partial<ScopeContext>): string => { /*...*/ },
message: (message: string, captureContext?: Partial<ScopeContext>): string => { /*...*/ },
wrapHandler: (handler: AWSLambda.Handler): AWSLambda.Handler => {
return Sentry.AWSLambda.wrapHandler(async (event, context, callback) => {
try {
return await handler(event, context, callback);
} catch (error) {
console.log(`Error: ${errorToString(error)}`);
if (error instanceof MetriportError && error.additionalInfo) {
capture.setExtra(error.additionalInfo);
}
throw error;
}
});
},
};- The lambda handler in
hl7-notification-webhook-sender.tsis wrapped withcapture.wrapHandler. - This wrapper logs and rethrows errors, which means any error thrown inside the handler or called functions will propagate and cause the lambda to fail.
File: packages/core/src/command/hl7-notification/hl7-notification-webhook-sender-direct.ts
Lines 29-146 (summary):
export class Hl7NotificationWebhookSenderDirect implements Hl7NotificationWebhookSender {
constructor(
private readonly apiUrl: string,
private readonly oldBucketName: string,
private readonly bucketName: string,
private readonly s3Utils = new S3Utils(Config.getAWSRegion())
) {}
async execute(params: Hl7Notification): Promise<void> {
// Parses HL7 message, validates type, fetches patient data from API,
// converts HL7 to FHIR bundle, uploads bundles to S3 buckets,
// gets presigned URLs, and notifies internal API via HTTP POST.
// Uses executeWithNetworkRetries for network calls, which may throw on failure.
// Throws if unsupported trigger event or if any network or S3 operation fails.
}
}- The
executemethod called in the lambda handler performs multiple network and S3 operations. - It throws errors if:
- The HL7 message trigger event is unsupported.
- Network requests fail (e.g., fetching patient data, notifying API).
- Uploading to S3 fails.
- These thrown errors will propagate back to the lambda handler and cause it to throw.
Summary of relevance:
- The lambda handler calls
getSingleMessageOrFailwhich throws if the SQS event has zero or multiple messages. - The handler calls
Hl7NotificationWebhookSenderDirect.executewhich throws on various failure conditions (network, unsupported message, S3 upload). - The handler is wrapped with
capture.wrapHandlerwhich logs and rethrows any errors. - Therefore, the implementation will throw exceptions in error scenarios, consistent with the user comment "their implementation will throw".
These snippets provide the necessary context to understand where and why exceptions may be thrown in the hl7-notification-webhook-sender.ts lambda.
| let hl7ConversionBucket: s3.Bucket | undefined; | ||
| if (!isSandbox(props.config) && props.config.hl7Notification.hl7ConversionBucketName) { | ||
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | ||
| hl7ConversionBucket = new s3.Bucket(this, "HL7ConversionBucket", { | ||
| bucketName: props.config.hl7Notification.hl7ConversionBucketName, | ||
| publicReadAccess: false, | ||
| encryption: s3.BucketEncryption.S3_MANAGED, | ||
| versioned: true, | ||
| }); | ||
| } |
There was a problem hiding this comment.
Null-safety: guard against missing hl7Notification
If props.config.hl7Notification is undefined, dereferencing it (.hl7ConversionBucketName) will throw during synthesis.
Use optional chaining (or a separate guard) to keep CDK synthesis safe.
-if (!isSandbox(props.config) && props.config.hl7Notification.hl7ConversionBucketName) {
+if (
+ !isSandbox(props.config) &&
+ props.config.hl7Notification?.hl7ConversionBucketName
+) {📝 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.
| let hl7ConversionBucket: s3.Bucket | undefined; | |
| if (!isSandbox(props.config) && props.config.hl7Notification.hl7ConversionBucketName) { | |
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | |
| hl7ConversionBucket = new s3.Bucket(this, "HL7ConversionBucket", { | |
| bucketName: props.config.hl7Notification.hl7ConversionBucketName, | |
| publicReadAccess: false, | |
| encryption: s3.BucketEncryption.S3_MANAGED, | |
| versioned: true, | |
| }); | |
| } | |
| let hl7ConversionBucket: s3.Bucket | undefined; | |
| if ( | |
| !isSandbox(props.config) && | |
| props.config.hl7Notification?.hl7ConversionBucketName | |
| ) { | |
| // eslint-disable-next-line @typescript-eslint/no-unused-vars | |
| hl7ConversionBucket = new s3.Bucket(this, "HL7ConversionBucket", { | |
| bucketName: props.config.hl7Notification.hl7ConversionBucketName, | |
| publicReadAccess: false, | |
| encryption: s3.BucketEncryption.S3_MANAGED, | |
| versioned: true, | |
| }); | |
| } |
🤖 Prompt for AI Agents
In packages/infra/lib/api-stack.ts around lines 301 to 310, the code accesses
props.config.hl7Notification.hl7ConversionBucketName without checking if
hl7Notification is defined, which can cause a runtime error during synthesis.
Fix this by adding an optional chaining operator or a separate guard to ensure
props.config.hl7Notification is defined before accessing
hl7ConversionBucketName, preventing potential null or undefined dereference
errors.
There was a problem hiding this comment.
This condition could drop the !isSandbox completely, but we're shipping without changing this. Thank you anyway
There was a problem hiding this comment.
@lucasdellabella Understood! Thanks for considering the suggestion. Good luck with the release!
| // Upload to old bucket | ||
| const oldBundleFilename = buildHl7MessageFhirBundleFileKey({ | ||
| cxId, | ||
| patientId, | ||
| timestamp: sourceTimestamp, | ||
| messageId: getMessageUniqueIdentifier(message), | ||
| messageCode, | ||
| triggerEvent, | ||
| }); | ||
|
|
||
| await storeInS3WithRetries({ | ||
| ...nonSpecificUploadParams, | ||
| bucketName: this.oldBucketName, | ||
| payload: JSON.stringify(bundle), | ||
| fileName: newBundleFileName, | ||
| fileName: oldBundleFilename, | ||
| }); | ||
| log(`Uploaded to S3 bucket: ${this.oldBucketName}. Filepath: ${oldBundleFilename}`); | ||
|
|
There was a problem hiding this comment.
Logging potentially sensitive identifiers
oldBundleFilename almost certainly embeds cxId and patientId; logging the full key leaks PHI to CloudWatch/Sentry.
Mask or hash patient-specific segments before logging to stay HIPAA-compliant.
-log(`Uploaded to S3 bucket: ${this.oldBucketName}. Filepath: ${oldBundleFilename}`);
+log(
+ `Uploaded to S3 bucket: ${this.oldBucketName}. Filepath: ${oldBundleFilename.replace(
+ /[0-9a-f-]{36}/g,
+ "[uuid]"
+ )}`
+);Apply the same redaction to the second log statement below.
[security]
📝 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.
| // Upload to old bucket | |
| const oldBundleFilename = buildHl7MessageFhirBundleFileKey({ | |
| cxId, | |
| patientId, | |
| timestamp: sourceTimestamp, | |
| messageId: getMessageUniqueIdentifier(message), | |
| messageCode, | |
| triggerEvent, | |
| }); | |
| await storeInS3WithRetries({ | |
| ...nonSpecificUploadParams, | |
| bucketName: this.oldBucketName, | |
| payload: JSON.stringify(bundle), | |
| fileName: newBundleFileName, | |
| fileName: oldBundleFilename, | |
| }); | |
| log(`Uploaded to S3 bucket: ${this.oldBucketName}. Filepath: ${oldBundleFilename}`); | |
| // Upload to old bucket | |
| const oldBundleFilename = buildHl7MessageFhirBundleFileKey({ | |
| cxId, | |
| patientId, | |
| timestamp: sourceTimestamp, | |
| messageId: getMessageUniqueIdentifier(message), | |
| messageCode, | |
| triggerEvent, | |
| }); | |
| await storeInS3WithRetries({ | |
| ...nonSpecificUploadParams, | |
| bucketName: this.oldBucketName, | |
| payload: JSON.stringify(bundle), | |
| fileName: oldBundleFilename, | |
| }); | |
| log( | |
| `Uploaded to S3 bucket: ${this.oldBucketName}. Filepath: ${oldBundleFilename.replace( | |
| /[0-9a-f-]{36}/g, | |
| "[uuid]" | |
| )}` | |
| ); |
🤖 Prompt for AI Agents
In
packages/core/src/command/hl7-notification/hl7-notification-webhook-sender-direct.ts
around lines 88 to 104, the log statement outputs the full oldBundleFilename
which includes sensitive identifiers like cxId and patientId, risking PHI
exposure. To fix this, mask or hash the patient-specific parts of
oldBundleFilename before logging it to ensure HIPAA compliance. Apply the same
masking or hashing approach to the second log statement that logs the new bundle
filename as well.
There was a problem hiding this comment.
Why would this be leaking PHI?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the implementation of the file key builder functions
cat packages/core/src/command/hl7v2-subscriptions/hl7v2-to-fhir-conversion/shared.ts
Length of output: 5815
@lucasdellabella Based on examining the buildHl7MessageFhirBundleFileKey function, the oldBundleFilename contains directly identifiable PHI:
-
The file key structure is:
${cxId}/${patientId}/${timestamp}_${messageId}_${messageCode}_${triggerEvent}.${HL7_FILE_EXTENSION}.${JSON_FILE_EXTENSION} -
This means it contains:
- Patient IDs (direct PHI under HIPAA)
- Customer IDs (which may be considered PHI when linked to patient data)
- Timestamps of healthcare events
When this full path is logged to CloudWatch or Sentry, it creates permanent records containing identifiable patient data that could be accessed by personnel who don't need access to PHI.
The suggested replacement using regex to mask UUIDs would keep the log structure useful for debugging while removing the protected identifiers.
🧬 Code Graph Analysis Results
Relevant snippets for understanding hl7-notification-webhook-sender-direct.ts and the PHI leakage concern:
File: packages/core/src/command/hl7-notification/hl7-notification-webhook-sender.ts
Lines 1-11:
export interface Hl7NotificationWebhookSender {
execute(request: Hl7Notification): Promise<void>;
}
export type Hl7Notification = {
cxId: string;
patientId: string;
message: string;
messageReceivedTimestamp: string;
sourceTimestamp: string;
};- Defines the interface
Hl7NotificationWebhookSenderwith anexecutemethod taking anHl7Notificationobject. Hl7Notificationincludes identifiers (cxId,patientId), the raw HL7 message string, and timestamps.
File: packages/core/src/command/hl7v2-subscriptions/hl7v2-to-fhir-conversion/index.ts
Lines 22-68:
export function convertHl7v2MessageToFhir({
cxId,
patientId,
message,
timestampString,
}: Hl7ToFhirParams): Bundle<Resource> {
const { log } = out(`hl7v2 to fhir - cx: ${cxId}, pt: ${patientId}`);
log("Beginning conversion.");
const startedAt = new Date();
const { messageCode, triggerEvent } = getHl7MessageTypeOrFail(message);
const messageId = getMessageUniqueIdentifier(message);
const filePath = buildHl7MessageFileKey({
cxId,
patientId,
timestamp: timestampString,
messageId,
messageCode,
triggerEvent,
});
if (messageCode === "ADT") {
const resources = mapEncounterAndRelatedResources(message, patientId);
const bundle = buildBundleFromResources({ type: "collection", resources });
const duration = elapsedTimeFromNow(startedAt);
log(`Conversion completed in ${duration} ms`);
return addHl7SourceExtension(bundle, filePath);
}
const msg = "HL7 message type isn't supported";
log(`${msg} ${messageCode}. Skipping conversion.`);
const extraProps = {
patientId,
cxId,
messageCode,
};
capture.message(msg, {
extra: extraProps,
level: "info",
});
throw new MetriportError(msg, undefined, extraProps);
}- Converts HL7 v2 messages to FHIR bundles.
- Only supports ADT message type for conversion.
- Returns a FHIR
Bundle<Resource>containing patient and encounter data extracted from the HL7 message.
File: packages/core/src/external/fhir/shared/bundle.ts
Lines 154-162 and 187-193:
export function buildBundle({
type = "searchset",
entries = [],
}: {
type?: Bundle["type"];
entries?: BundleEntry[];
} = {}): Bundle {
return { resourceType: "Bundle", total: entries.length, type, entry: entries };
}
export const buildBundleEntry = <T extends Resource>(resource: T): BundleEntry<T> => {
const fullUrl = buildFullUrl(resource);
return {
...(fullUrl ? { fullUrl } : {}),
resource,
};
};- Utility functions to build FHIR bundles and bundle entries from FHIR resources.
File: packages/core/src/external/aws/s3.ts
Summary of relevant parts:
S3Utilsclass provides methods to interact with AWS S3, including uploading files, generating signed URLs, and checking file existence.storeInS3WithRetriesfunction uploads a payload to S3 with retry logic and error handling.getSignedUrlmethod generates presigned URLs for accessing S3 objects with limited time validity.
File: packages/core/src/command/hl7v2-subscriptions/hl7v2-to-fhir-conversion/msh.ts
Lines 18-35 and 45-47:
export function getHl7MessageTypeOrFail(hl7Message: Hl7Message): Hl7MessageType {
const mshSegment = getSegmentByNameOrFail(hl7Message, "MSH");
const messageType = getOptionalValueFromSegment(mshSegment, MSH_9_MESSAGE_TYPE, 1);
const triggerEvent = getOptionalValueFromSegment(mshSegment, MSH_9_MESSAGE_TYPE, 2);
const messageStructure = getOptionalValueFromSegment(mshSegment, MSH_9_MESSAGE_TYPE, 3);
if (!messageType) {
throw new MetriportError("Message type not found in MSH segment");
}
const derivedTriggerEvent = triggerEvent ?? messageStructure?.split("_")[1];
if (!derivedTriggerEvent) {
throw new MetriportError("Trigger event not found in MSH segment");
}
return { messageCode: messageType, triggerEvent: derivedTriggerEvent };
}
export function getMessageUniqueIdentifier(msg: Hl7Message): string {
return getOptionalValueFromMessage(msg, "MSH", 10, 1) ?? createUuidFromText(msg.toString());
}- Extracts HL7 message type and trigger event from the MSH segment.
- Retrieves a unique message identifier from the HL7 message.
File: packages/core/src/command/hl7v2-subscriptions/hl7v2-to-fhir-conversion/adt/utils.ts
Lines 28-36:
export function getEncounterPeriod(adt: Hl7Message): Encounter["period"] | undefined {
const pv1Segment = adt.getSegment("PV1");
if (!pv1Segment) return undefined;
const start = getOptionalValueFromSegment(pv1Segment, 44, 1);
const end = getOptionalValueFromSegment(pv1Segment, 45, 1);
return buildPeriod(start, end);
}- Extracts encounter start and end timestamps from the PV1 segment of an ADT HL7 message.
File: packages/core/src/command/hl7v2-subscriptions/hl7v2-to-fhir-conversion/shared.ts
Lines 170-183:
export function buildHl7MessageFhirBundleFileKey(params: Hl7FileKeyParams) {
return `${buildHl7MessageFileKey(params)}.${JSON_FILE_EXTENSION}`;
}
export function buildHl7MessageConversionFileKey({
cxId,
patientId,
messageId,
timestamp,
messageCode,
triggerEvent,
}: Hl7FileKeyParams) {
return `cxId=${cxId}/ptId=${patientId}/${messageCode.toUpperCase()}/${timestamp}_${messageId}_${messageCode}_${triggerEvent}.${HL7_FILE_EXTENSION}.${JSON_FILE_EXTENSION}`;
}- Functions to build S3 file keys for storing HL7 message conversion results and FHIR bundles.
File: packages/core/src/util/mime.ts
Line 17:
export const JSON_APP_MIME_TYPE = "application/json";- Defines the MIME type used for JSON payloads uploaded to S3.
Summary of relevant context:
- The
Hl7NotificationWebhookSenderDirectclass parses HL7 messages, converts supported ADT messages to FHIR bundles including patient data, and uploads these bundles to S3 buckets. - It also generates presigned URLs for these bundles and notifies an internal API endpoint with the URL and metadata.
- The uploaded bundles contain FHIR resources derived from the HL7 message, including patient information.
- The S3 upload uses JSON content type and retries on failure.
- The patient data is fetched from an internal API and added to the FHIR bundle before upload.
- The HL7 message and converted FHIR bundle potentially contain Protected Health Information (PHI) such as patient identifiers, encounter timestamps, and clinical data.
- The presigned URLs provide temporary access to these bundles stored in S3.
This context is relevant to understanding why PHI might be leaked: the code uploads detailed patient-related data to S3 and shares presigned URLs externally or internally, which if mishandled, could expose PHI.
✏️ Learnings added
Learnt from: lucasdellabella
PR: metriport/metriport#3855
File: packages/core/src/command/hl7-notification/hl7-notification-webhook-sender-direct.ts:88-104
Timestamp: 2025-05-16T02:10:01.775Z
Learning: When logging file paths or other strings that might contain patient identifiers like patientId or cxId, these should be masked or redacted (e.g., using regex replacement) to avoid logging Protected Health Information (PHI) to CloudWatch or error tracking systems like Sentry, ensuring HIPAA compliance.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
There was a problem hiding this comment.
We have BAAs with these systems, and while trying to avoid sending PHI anywhere at all, this is extremely minimal and needed to help us swiftly triage and resolve production issues, so im opting to leave it in.
There was a problem hiding this comment.
@lucasdellabella I understand your decision. Given you have BAAs with these systems and need this information for production issue resolution, I respect your choice to keep the full paths in logs. Thanks for explaining your rationale.
🧠 Learnings used
Learnt from: lucasdellabella
PR: metriport/metriport#3855
File: packages/core/src/command/hl7-notification/hl7-notification-webhook-sender-direct.ts:88-104
Timestamp: 2025-05-16T02:10:01.775Z
Learning: When logging file paths or other strings that might contain patient identifiers like patientId or cxId, these should be masked or redacted (e.g., using regex replacement) to avoid logging Protected Health Information (PHI) to CloudWatch or error tracking systems like Sentry, ensuring HIPAA compliance.
Part of cs-465 Signed-off-by: Jorge Orta <36802074+Orta21@users.noreply.github.com>
Part of cs-465 Signed-off-by: Jorge Orta <36802074+Orta21@users.noreply.github.com>
build: Tweaks to the PCP Summary
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/core/src/command/ai-brief/filter.ts (1)
503-548: Good implementation of Observation filtering logic.The function effectively groups and filters observations by their reading codes, keeping only the most recent values. This follows the functional programming principles recommended in the guidelines: it's immutable, deterministic, and has clear inputs and outputs.
However, consider adding a brief comment explaining the edge case handling in the date comparison (lines 534-541).
const latest = obsGroup.sort((a, b) => { const aDate = getObservationDate(a); const bDate = getObservationDate(b); + // Sort by date descending, handling cases where dates may be missing if (aDate && bDate) { return bDate.localeCompare(aDate); } if (aDate && !bDate) return -1; if (!aDate && bDate) return 1; return 0; })[0];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Lite
📒 Files selected for processing (2)
packages/core/src/command/ai-brief/filter.ts(5 hunks)packages/core/src/command/ai-brief/pcp-visit-prompt.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`**/*.ts`: - Use the Onion Pattern to organize a package's code in layers - Try to use immutable code and avoid sharing state across different functions, objects, and systems - Try...
**/*.ts: - Use the Onion Pattern to organize a package's code in layers
- Try to use immutable code and avoid sharing state across different functions, objects, and systems
- Try to build code that's idempotent whenever possible
- Prefer functional programming style functions: small, deterministic, 1 input, 1 output
- Minimize coupling / dependencies
- Avoid modifying objects received as parameter
- Only add comments to code to explain why something was done, not how it works
- Naming
- classes, enums:
PascalCase- constants, variables, functions:
camelCase- file names:
kebab-case- table and column names:
snake_case- Use meaningful names, so whoever is reading the code understands what it means
- Don’t use negative names, like
notEnabled, preferisDisabled- For numeric values, if the type doesn’t convey the unit, add the unit to the name
- Typescript
- Use types
- Prefer
constinstead oflet- Avoid
anyand casting fromanyto other types- Type predicates: only applicable to narrow down the type, not to force a complete type conversion
- Prefer deconstructing parameters for functions instead of multiple parameters that might be of
the same type- Don’t use
nullinside the app, only on code interacting with external interfaces/services,
like DB and HTTP; convert toundefinedbefore sending inwards into the code- Use
async/awaitinstead of.then()- Use the strict equality operator
===, don’t use abstract equality operator==- When calling a Promise-returning function asynchronously (i.e., not awaiting), use
.catch()to
handle errors (seeprocessAsyncErrorandemptyFunctiondepending on the case)- Date and Time
- Always use
buildDayjs()to createdayjsinstances- Prefer
dayjs.duration(...)to create duration consts and keep them asduration- Prefer Nullish Coalesce (??) than the OR operator (||) to provide a default value
- Avoid creating arrow functions
- Use truthy syntax instead of
in- i.e.,if (data.link)notif ('link' in data)- Error handling
- Pass the original error as the new one’s
causeso the stack trace is persisted- Error messages should have a static message - add dynamic data to MetriportError's
additionalInfoprop- Avoid sending multiple events to Sentry for a single error
- Global constants and variables
- Move literals to constants declared after imports when possible (avoid magic numbers)
- Avoid shared, global objects
- Avoid using
console.logandconsole.errorin packages other than utils, infra and shared,
and try to useout().loginstead- Avoid multi-line logs
- don't send objects as a second parameter to
console.log()orout().log()- don't create multi-line strings when using
JSON.stringify()- Use
eslintto enforce code style- Use
prettierto format code- max column length is 100 chars
- multi-line comments use
/** */- scripts: top-level comments go after the import
packages/core/src/command/ai-brief/filter.tspackages/core/src/command/ai-brief/pcp-visit-prompt.ts
🧬 Code Graph Analysis (1)
packages/core/src/command/ai-brief/filter.ts (1)
packages/core/src/domain/ai-brief/modify-resources.ts (2)
SlimResource(62-76)SlimObservation(394-405)
🔇 Additional comments (16)
packages/core/src/command/ai-brief/filter.ts (5)
7-7: Missing import needs to be documented in the AI summary.The AI summary doesn't mention the addition of the
SlimObservationimport, which is now used in the new filtering functions.Likely an incorrect or invalid review comment.
23-29: Good documentation for the reference resources list.The JSDoc provides a clear explanation of what reference resources are and why they're filtered out, improving code readability and maintainability.
105-105: Well-integrated filtering step enhances data quality.The new filtering step ensures only the most recent Observation per code is included in the final payload, reducing redundancy while preserving essential clinical information.
114-114: Variable name updated for clarity.The variable name change from
withFilteredReportstowithLatestObservationsproperly reflects the transformation applied by the new filtering function.
550-556: Clear helper function for date extraction.This function follows the single responsibility principle and has a descriptive name. It handles the extraction of the most relevant date from an observation, providing a clean abstraction for the sorting logic.
packages/core/src/command/ai-brief/pcp-visit-prompt.ts (11)
3-3: Enhanced privacy protection in system prompt.The updated system prompt explicitly prohibits including demographic information and personal identifiers, which is an important privacy improvement.
9-15: Improved formatting instructions and PCP visit criteria.The enhancements provide clearer guidance for consistent output formatting and expand PCP visit identification criteria to include all office visits, outpatient visits, and established patient visits with specific provider types and specialties.
16-16: Simplified Medical/Problem List format.Changing to a binary "Has Conditions: Y/N" format simplifies the output while still indicating the presence of medical conditions.
20-20: Improved BMI labeling.The label now includes both the full term "Body mass index" and the abbreviation "BMI", improving clarity.
24-26: Added timeframe requirements for medications.Specifying that medications should only be included if they have fill dates within the last 60 days ensures the summary contains only current medications, improving clinical relevance.
27-33: Added "Physician Team Only Indicators" section.This new section highlights specific conditions and medications that are particularly important for the medical team, enhancing the clinical value of the summary.
34-40: Detailed PCP visit identification instructions.The explicit instructions for identifying PCP visits and provider specialties will lead to more consistent and accurate identification of relevant visits in the summary.
54-56: Clarified disqualifying conditions criteria.Adding "active" as a qualifier for cancer and drug/alcohol abuse provides more specific criteria for excluding patients, improving clinical decision-making.
61-61: Added timeframe for qualifying medications.Specifying the 60-day prescription window ensures only current medications are considered qualifying, improving clinical relevance.
78-80: Clarified disqualifying medications criteria.Adding the timeframe constraint for disqualifying medications ensures consistency with the qualifying medications criteria.
81-81: Important note on not inferring conditions.This note prevents the AI from overreaching by inferring conditions from lab values or observations, requiring that only explicitly documented conditions and medications be included.
Issues:
Dependencies
Description
Testing
Check each PR.
Release Plan
Summary by CodeRabbit
Summary by CodeRabbit