packages/core/package.json (1)

128-129: aws-sdk v2 usage detected in core — retain dependency and track migration

We still import from aws-sdk v2 across multiple modules, so you should keep the v2 dependency for now. Please add TODOs or file a tracking issue to migrate each of these to the v3 client:

• packages/core/src/command/consolidated/get-snapshot-lambda.ts
• packages/core/src/external/aws/s3.ts
• packages/core/src/external/aws/dynamodb.ts
• packages/core/src/external/aws/lambda.ts
• packages/core/src/external/commonwell/document/document-downloader-local.ts
• packages/core/src/external/carequality/ihe-gateway-v2/saml/saml-client.ts

…(and other aws-sdk imports identified by your grep)

Once migration is complete, remove the aws-sdk v2 entry from package.json to slim installs.

packages/core/src/command/analytics-platform/merge-csvs/index.ts (1)

10-13: Consider making concurrency limits configurable via environment variables

The hard-coded concurrency limits numberOfParallelListObjectsFromS3 and numberOfParallelMergeFileGroups should be configurable via environment variables to allow runtime tuning without code changes, especially given the performance and memory implications mentioned in the comments.

Based on the past review comment by leite08, this has been noted as something that should probably be moved to environment variables via CDK.

packages/utils/src/fhir/bundle/count_resources.sh (3)

78-83: Ensure .entry is an array before processing

The current check passes when .entry is any non-null value. Tighten it to ensure it’s an array to avoid jq errors and unnecessary stderr handling downstream.

Apply this diff:

-    # Check if file is valid JSON and has .entry array
-    if ! jq -e '.entry' "$file" >/dev/null 2>&1; then
+    # Check if file is valid JSON and has .entry array
+    if ! jq -e 'has("entry") and (.entry | type == "array")' "$file" >/dev/null 2>&1; then
         echo "  No .entry array found in this file (or invalid JSON)"
         continue
     fi

---

109-110: Nit: remove trailing whitespace

Minor formatting nit: there is a trailing space after "Done.".

Apply this diff:

-echo "Done." 
+echo "Done."

---

47-51: Optional: compute all counts in a single jq pass for performance on large files

Current approach invokes jq once per resource type. For very large bundles, a single-pass jq can be significantly faster.

If you want, I can provide a drop-in jq that emits the same per-type counts and total in one call per file.

Also applies to: 92-106

packages/utils/src/fhir/bundle/find_with_custom_filter.sh (1)

112-118: Portability and space-safe iteration; avoid find -maxdepth on macOS

• find ... -maxdepth is not supported by BSD/macOS find.
• Iterating over a whitespace-delimited variable breaks on filenames with spaces.

Use globbing for depth=1 and fall back to find for deeper depths; iterate over an array.

Please validate behavior for MAXDEPTH=1 on macOS and for deeper depths on Linux.

Apply this diff:

-# Find all JSON files in the directory and subdirectories up to maxdepth
-json_files=$(find "$DIRECTORY" -maxdepth "$MAXDEPTH" -name "*.json" -type f | sort)
-
-if [ -z "$json_files" ]; then
-    echo "No JSON files found in directory: $DIRECTORY"
-    exit 0
-fi
+# Build file list (portable and space-safe)
+shopt -s nullglob
+files=()
+if [ "$MAXDEPTH" = "1" ]; then
+    files=("$DIRECTORY"/*.json)
+else
+    # Note: -maxdepth may not be available on BSD/macOS find; this is best-effort.
+    # On such systems, consider setting --maxdepth 1 or installing GNU find (gfind).
+    while IFS= read -r -d '' f; do files+=("$f"); done < <(find "$DIRECTORY" -name "*.json" -type f -print0 2>/dev/null)
+    IFS=$'\n' files=($(sort <<<"${files[*]}")); unset IFS
+fi
+
+if [ ${#files[@]} -eq 0 ]; then
+    echo "No JSON files found in directory: $DIRECTORY"
+    exit 0
+fi
@@
-# Process each JSON file
-for file in $json_files; do
+# Process each JSON file
+for file in "${files[@]}"; do
     # Get relative path from the search directory for better identification
     if [ "$DIRECTORY" = "." ]; then
         filename="$file"
     else
         filename="${file#$DIRECTORY/}"
     fi
@@
-    # Check if file is valid JSON and has .entry array
-    if ! jq -e '.entry' "$file" >/dev/null 2>&1; then
+    # Check if file is valid JSON and has .entry array
+    if ! jq -e 'has("entry") and (.entry | type == "array")' "$file" >/dev/null 2>&1; then
         continue
     fi

Also applies to: 129-147

packages/utils/src/consolidated/bulk-recreate-consolidated.ts (2)

25-28: Comment suggests ≥1s minimum delay, but code allows 500ms — align wording or constants.

Docs say we “likely don't want to get the delay/sleep under 1 second,” while minimumDelayTime is 500ms. Either bump the minimum delay to 1s, or soften/clarify the wording here to reflect the current minimum.

Here’s a wording tweak that keeps flexibility and guides readers to the constants:

- * IMPORTANT: while the endpoint doesn't seem to do much work with shared resources (mostly S3 and
- * lambda), when consolidated get re-created we re-ingest the patient's data into OpenSearch (OS).
- * Be mindful about that, so we likely don't want to get the delay/sleep under 1 second.
- *
+ * IMPORTANT: while the endpoint doesn't do much work with shared resources (mostly S3 and Lambda),
+ * when consolidated are re-created we re-ingest the patient's data into OpenSearch (OS).
+ * Be mindful: we generally shouldn't go below ~1 second between requests. If you need to tweak the
+ * rate, review the minimum/default delay constants below.
+ *

---

58-60: Consider raising the minimum delay to 1s to match the caution around OpenSearch load.

Given the note about OS re-ingestion and the default of 2s, a 1s minimum would better enforce the operational safety margin you described. If you prefer to keep flexibility, the comment in Lines 25–28 should be updated instead (see prior comment).

-const minimumDelayTime = dayjs.duration(500, "milliseconds");
+const minimumDelayTime = dayjs.duration(1, "second");
 const defaultDelayTime = dayjs.duration(2, "seconds");

packages/lambdas/package.json (2)

21-23: Align Lambda Node.js engine to supported runtimes (>=18, ideally 20).

Lambda no longer supports Node.js 16. Given the addition of AWS SDK v3 and snowflake-sdk, bump the engines requirement to >=18 (preferably >=20) to match deployed runtimes and avoid accidental local usage of unsupported versions.

Apply this diff:

-  "engines": {
-    "node": ">=16"
-  },
+  "engines": {
+    "node": ">=18"
+  },

---

31-31: Mixing AWS SDK v3 minor versions across packages; consider harmonizing to reduce duplication.

You’re adding @aws-sdk/client-lambda at ^3.865.0 while other AWS SDK v3 deps are ~3.800.x. Not a blocker, but aligning versions across v3 packages helps dedupe and shrinks bundles.

Would you like a follow-up PR to harmonize v3 package versions across the repo?

packages/utils/package.json (2)

74-74: ini addition looks fine; ensure consistent parsing and sanitization at call sites.

No action needed here; just a reminder to avoid writing parsed INI objects back to untrusted sinks.

---

81-83: snowflake-sdk and matching type packages are correctly added for the Step 3 CLI.

Good call adding @types/snowflake-sdk. Ensure any OCSP/network settings used by the CLI are documented for local runs.

packages/core/src/external/aws/lambda-logic/README.md (1)

3-7: Fix grammar and tighten the deprecation message.

Polish wording and clarify guidance to point developers to per-use-case folders.

Apply this diff:

-This folder should not exist. If we put all "lambda logic" in this folder, we will have a hard time
-maintaining it, all lambdas' logic will be in a single place.
-
-What we've been doing for all other lambdas, is to call commands/functions that are stored on the
-proper folder for each use case/business logic.
+This folder should not exist. Centralizing all “lambda logic” here makes maintenance harder because
+every lambda’s implementation ends up in a single place.
+
+Instead, follow the established pattern: place code under the appropriate per–use-case folder and
+invoke commands/functions from there.

packages/utils/src/analytics-platform/3.readme.md (1)

1-1: Tighten wording and fix hyphenation.

Use “subfolder” (closed compound) and clarify punctuation.

Apply this diff:

-The 3rd step is platform-specific, check the respective sub-folder for a script starting with "3-".
+The 3rd step is platform-specific; check the respective subfolder for a script starting with "3-".

packages/data-transformation/fhir-to-csv/main.py (1)

127-130: Follow through: remove or lazy-load Snowflake bits and update messaging.

Since Snowflake interaction is disabled here:

• Consider removing or lazy-loading snowflake.connector and src.setupSnowflake imports to cut cold-start time and reduce unused dependencies.
• Update the log on Line 131 to avoid “for Snowflake” wording (it’s misleading now).

Example (outside this hunk) for the log message:

print(f">>> Parsing data and uploading it to S3 - {cx_id}, patient_id {patient_id}, job_id {job_id}")

packages/infra/lib/shared/settings.ts (1)

6-13: DRY the memory union via a shared type.

To avoid future divergence, factor the memory union into a single alias and reuse it in both LambdaSettings and QueueAndLambdaSettings.lambda.

Example:

+type LambdaMemory = 512 | 1024 | 2048 | 4096 | 6144 | 8192 | 10240;
 export type LambdaSettings = {
-  memory: 512 | 1024 | 2048 | 4096 | 6144 | 8192 | 10240;
+  memory: LambdaMemory;
   /** How long can the lambda run for, max is 900 seconds (15 minutes)  */
   timeout: Duration;
   reservedConcurrentExecutions?: number;
   ephemeralStorageSize?: Size;
   runtime: Runtime;
 };

 // TODO Unify w/ LambdaSettings
   lambda: {
-    memory: 512 | 1024 | 2048 | 4096 | 6144 | 8192 | 10240;
+    memory: LambdaMemory;
     /** How long can the lambda run for, max is 900 seconds (15 minutes)  */
     timeout: Duration;
     reservedConcurrentExecutions?: number;
     ephemeralStorageSize?: Size;
   };

Also applies to: 21-26

packages/core/src/external/aws/s3.ts (1)

571-599: Normalize prefix to ensure true “first-level” semantics and avoid surprises.

The pagination and Delimiter usage look good. To make results consistent when callers pass a prefix without a trailing delimiter, normalize it before listing. That avoids mixing “sibling” prefixes with similarly named directories.

Apply this diff to sanitize the prefix:

   async listFirstLevelSubdirectories({
     bucket,
     prefix,
     delimiter = "/",
   }: {
     bucket: string;
     prefix: string;
     delimiter?: string | undefined;
   }): Promise<AWS.S3.CommonPrefixList> {
-    const allObjects: AWS.S3.CommonPrefix[] = [];
+    const allObjects: AWS.S3.CommonPrefix[] = [];
+    const sanitizedPrefix = prefix.endsWith(delimiter) ? prefix : `${prefix}${delimiter}`;
     let continuationToken: string | undefined;
     do {
       const res = await executeWithRetriesS3(() =>
         this._s3
           .listObjectsV2({
             Bucket: bucket,
-            Prefix: prefix,
+            Prefix: sanitizedPrefix,
             ...(delimiter ? { Delimiter: delimiter } : {}),
             ...(continuationToken ? { ContinuationToken: continuationToken } : {}),
           })
           .promise()
       );

packages/utils/src/analytics-platform/1-fhir-to-csv.ts (1)

1-1: Move the header comment below imports per scripts guideline.

Our scripts guideline says top-level comments go after imports. Consider moving the “1-fhir-to-csv.ts” header comment below the import block.

packages/core/src/external/snowflake/commands.ts (2)

3-15: Consider adding connection state validation and timeout configuration.

The promisified connection functions don't validate the connection state before attempting operations. Additionally, there's no timeout configuration for these operations which could lead to indefinite hangs.

Consider adding connection state validation and optional timeout support:

-export function promisifyConnect(s: snowflake.Connection) {
-  return function (): Promise<snowflake.Connection> {
+export function promisifyConnect(s: snowflake.Connection, timeoutMs?: number) {
+  return function (): Promise<snowflake.Connection> {
     return new Promise((resolve, reject) => {
+      const timeout = timeoutMs ? setTimeout(() => {
+        reject(new Error(`Connection timeout after ${timeoutMs}ms`));
+      }, timeoutMs) : undefined;
+      
       s.connect((error: unknown, result: snowflake.Connection) => {
+        if (timeout) clearTimeout(timeout);
         if (error) {
           reject(error);
           return;
         }
         resolve(result);
       });
     });
   };
 }

---

31-53: Consider making the promisified execute function more type-safe.

The function uses any[] for rows which reduces type safety. Consider using generics to allow callers to specify the expected row type.

Apply this diff to improve type safety:

-export function promisifyExecute(s: snowflake.Connection) {
-  return function (sqlText: string): Promise<{
+export function promisifyExecute<T = any>(s: snowflake.Connection) {
+  return function (sqlText: string): Promise<{
     statement: snowflake.RowStatement;
-    rows: any[] | undefined; // eslint-disable-line @typescript-eslint/no-explicit-any
+    rows: T[] | undefined;
   }> {
     return new Promise((resolve, reject) => {
       s.execute({
         sqlText,
-        // eslint-disable-next-line @typescript-eslint/no-explicit-any
-        complete: (error: unknown, statement: snowflake.RowStatement, rows: any[] | undefined) => {
+        complete: (error: unknown, statement: snowflake.RowStatement, rows: T[] | undefined) => {
           if (error) {
             reject(error);
             return;
           }
           resolve({
             statement,
             rows: rows || undefined,
           });
         },
       });
     });
   };
 }

packages/utils/src/analytics-platform/util/check-duplicate-patient-ids.ts (1)

261-288: Consider using parseArgs for command-line argument handling.

For better CLI argument handling and validation, consider using Node.js's built-in parseArgs utility or a library like commander for more robust command-line parsing.

Example improvement using Node.js's parseArgs:

+import { parseArgs } from "node:util";
+
 async function main() {
   try {
-    // Get folder path from command line arguments
-    const args = process.argv.slice(2);
-
-    if (args.length === 0) {
+    const { values, positionals } = parseArgs({
+      allowPositionals: true,
+      strict: true
+    });
+    
+    if (positionals.length === 0) {
       console.log("Usage: ts-node check-duplicate-patient-ids.ts <folder-path>");
       // ... rest of usage
       process.exit(1);
     }
     
-    const folderPath = args[0];
+    const folderPath = positionals[0];

packages/utils/src/analytics-platform/snowflake/3-ingest-from-merged-csvs.ts (1)

85-132: Consider implementing connection pooling and retry logic.

The Snowflake connection is created once per execution without retry logic. Network issues or transient failures could cause the entire ingestion to fail.

Consider implementing:

1. Connection retry logic with exponential backoff
2. Connection pooling if multiple concurrent operations are planned
3. Checkpoint/resume capability to handle partial failures gracefully

This would make the ingestion process more resilient to transient failures and network issues.

packages/core/src/external/aws/lambda.ts (1)

100-100: Consider decoding the Payload using TextDecoder for V3.

In AWS SDK v3, the Payload is typically a Uint8Array that should be decoded using TextDecoder instead of toString().

Apply this diff for proper V3 payload decoding:

 export function getLambdaErrorV3(result: InvokeCommandOutput): LambdaError | undefined {
   if (!result.Payload) return undefined;
   if (!isLambdaErrorV3(result)) return undefined;
-  const response = JSON.parse(result.Payload.toString());
+  const decoder = new TextDecoder();
+  const response = JSON.parse(decoder.decode(result.Payload));
   return {
     errorType: response.errorType,
     errorMessage: response.errorMessage,
     log: logResultToString(result.LogResult),
   };
 }

Similarly for getLambdaResultPayloadV3:

-  return result.Payload.toString();
+  const decoder = new TextDecoder();
+  return decoder.decode(result.Payload);

packages/core/src/command/analytics-platform/fhir-to-csv/command/fhir-to-csv-transform.ts (1)

5-5: Clean up commented import

Remove the commented import statement as it creates code clutter. If this import is no longer needed, it should be deleted entirely.

-// import { getSnowflakeCreds } from "../../config";

packages/core/src/command/analytics-platform/merge-csvs/index.ts (1)

238-297: Greedy algorithm may produce suboptimal distribution for certain file sizes

The current greedy algorithm for distributing small files may lead to imbalanced groups when file sizes vary significantly. Consider implementing a more sophisticated bin packing algorithm (like First Fit Decreasing or Best Fit Decreasing) for better balance.

For example, when you have files of sizes [90MB, 10MB, 10MB, 10MB] with a 100MB target, the current algorithm might put 90MB+10MB in one group and 10MB+10MB in another, resulting in 100MB and 20MB groups. A better distribution would be [90MB, 10MB] and [10MB, 10MB] for two 100MB groups.

Consider using a more balanced approach:

-    // First pass: distribute large files evenly
-    for (let i = 0; i < sortedFiles.length; i++) {
-      const file = sortedFiles[i];
-      if (!file) continue;
-
-      // Find the group with the smallest current size
-      let smallestGroupIndex = 0;
-      let smallestGroupSize = groupSizes[0] || 0;
-
-      for (let j = 1; j < optimalGroupCount; j++) {
-        const currentSize = groupSizes[j] || 0;
-        if (currentSize < smallestGroupSize) {
-          smallestGroupSize = currentSize;
-          smallestGroupIndex = j;
-        }
-      }
-
-      // Add file to the smallest group
-      const targetGroup = groups[smallestGroupIndex];
-      if (targetGroup) {
-        targetGroup.push(file);
-        groupSizes[smallestGroupIndex] = (groupSizes[smallestGroupIndex] || 0) + file.size;
-      }
-    }
+    // Improved distribution using best-fit approach
+    for (const file of sortedFiles) {
+      // Find the best group: smallest that can still fit this file
+      let bestGroupIndex = -1;
+      let bestGroupSpace = Infinity;
+      
+      for (let j = 0; j < optimalGroupCount; j++) {
+        const currentSize = groupSizes[j] || 0;
+        const remainingSpace = targetGroupSizeBytes - currentSize;
+        
+        // If file fits and this group has less wasted space
+        if (file.size <= remainingSpace && remainingSpace < bestGroupSpace) {
+          bestGroupSpace = remainingSpace;
+          bestGroupIndex = j;
+        }
+      }
+      
+      // If no group can fit, add to smallest group
+      if (bestGroupIndex === -1) {
+        bestGroupIndex = groupSizes.indexOf(Math.min(...groupSizes));
+      }
+      
+      const targetGroup = groups[bestGroupIndex];
+      if (targetGroup) {
+        targetGroup.push(file);
+        groupSizes[bestGroupIndex] = (groupSizes[bestGroupIndex] || 0) + file.size;
+      }
+    }

packages/core/src/command/analytics-platform/merge-csvs/__tests__/index.test.ts (1)

1-1: Remove eslint-disable comment

The blanket disable of @typescript-eslint/no-non-null-assertion reduces type safety. Instead, handle the specific cases with proper null checks or use type guards.

-/* eslint-disable @typescript-eslint/no-non-null-assertion */
 import { FileInfo, groupFilesByTypeAndSize } from "../index";

Then update the specific assertions:

-      expect(largeFileGroup!.totalSize).toBe(largeFileSize);
+      expect(largeFileGroup?.totalSize).toBe(largeFileSize);

packages/infra/lib/api-stack.ts (1)

800-803: Fix JSDoc typo and clarify the deprecation note

There’s an extra backtick and the note could be clearer about what should move where.

Apply this diff:

-    /**
-     * @deprecated This should be done on the code where it's used.
-     * @see `setupFhirToCsvLambda()`` in the analytics-platform-stack.ts
-     */
+    /**
+     * @deprecated Move these bucket grants to the code/stack that owns the resource.
+     * @see setupFhirToCsvLambda() in analytics-platform-stack.ts
+     */

packages/infra/lib/analytics-platform/analytics-platform-stack.ts (5)

61-82: MergeCsvs concurrency/timeout are sensible; consider reserved concurrency and ephemeral storage (optional)

The 14m50s timeout and 4GB memory with batchSize=1 and maxConcurrency=20 looks fine for merging large CSVs. Two optional improvements:

• Set reservedConcurrentExecutions to match maxConcurrency to guard against other triggers or accidental invokes.
• If the implementation uses /tmp for intermediate files, consider ephemeralStorageSize (e.g., 2–10 GiB) to avoid ENOSPC under peak loads. If fully streaming, you’re fine as-is.

Apply one or both diffs:

Option A: restrict concurrency at the function level (tie to 20)

   const mergeCsvs: QueueAndLambdaSettings = {
     name: "MergeCsvs",
     entry: "analytics-platform/merge-csvs",
     lambda: {
       memory: 4096,
       timeout: mergeCsvsLambdaTimeout,
+      reservedConcurrentExecutions: 20,
     },

Option B: provision larger ephemeral storage (only if you write to /tmp)

   const mergeCsvs: QueueAndLambdaSettings = {
     name: "MergeCsvs",
     entry: "analytics-platform/merge-csvs",
     lambda: {
       memory: 4096,
       timeout: mergeCsvsLambdaTimeout,
+      ephemeralStorageSize: cdk.Size.gibibytes(2),
     },

---

109-114: Snowflake creds TODO: track it explicitly

“ENG-858 reintroduce this” is fine, but easy to lose. Suggest adding a link to the ticket in the comment and/or opening a follow-up issue to ensure the reintroduction is not forgotten.

Want me to open a follow-up item and annotate the code with the issue URL?

---

241-243: Comment drift: it says “medical document bucket” but you grant on the analytics bucket

The comment doesn’t match the code. You’re granting read/write to the analytics platform bucket here.

Apply this diff:

-    // Grant read to medical document bucket set on the api-stack
-    ownProps.analyticsPlatformBucket.grantReadWrite(fhirToCsvTransformLambda);
+    // Grant read/write to the analytics platform bucket
+    ownProps.analyticsPlatformBucket.grantReadWrite(fhirToCsvTransformLambda);

---

173-182: Drop unused props from setupMergeCsvsLambda call (optional)

awsRegion and config aren’t used by setupMergeCsvsLambda. Keeping only what’s needed reduces noise.

Apply this diff:

-    const { mergeCsvsLambda, queue: mergeCsvsQueue } = this.setupMergeCsvsLambda({
-      config: props.config,
-      envType: props.config.environmentType,
-      awsRegion: props.config.region,
-      lambdaLayers: props.lambdaLayers,
-      vpc: props.vpc,
-      sentryDsn: props.config.sentryDSN,
-      alarmAction: props.alarmAction,
-      bucket: analyticsPlatformBucket,
-    });
+    const { mergeCsvsLambda, queue: mergeCsvsQueue } = this.setupMergeCsvsLambda({
+      envType: props.config.environmentType,
+      lambdaLayers: props.lambdaLayers,
+      vpc: props.vpc,
+      sentryDsn: props.config.sentryDSN,
+      alarmAction: props.alarmAction,
+      bucket: analyticsPlatformBucket,
+    });

If you proceed, also update the method signature below.

---

290-299: Prune unused parameters from setupMergeCsvsLambda signature (optional)

config and awsRegion aren’t used. Easing the signature makes intent clearer.

Apply this diff:

-  private setupMergeCsvsLambda(ownProps: {
-    config: EnvConfigNonSandbox;
-    envType: EnvType;
-    awsRegion: string;
-    lambdaLayers: LambdaLayers;
-    vpc: ec2.IVpc;
-    sentryDsn: string | undefined;
-    alarmAction: SnsAction | undefined;
-    bucket: s3.Bucket;
-  }): {
+  private setupMergeCsvsLambda(ownProps: {
+    envType: EnvType;
+    lambdaLayers: LambdaLayers;
+    vpc: ec2.IVpc;
+    sentryDsn: string | undefined;
+    alarmAction: SnsAction | undefined;
+    bucket: s3.Bucket;
+  }): {

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled
• Linear integration is disabled

You can enable these sources in your CodeRabbit configuration.

Learnt from: leite08
PR: metriport/metriport#4255
File: packages/core/src/command/analytics-platform/fhir-to-csv/command/fhir-to-csv/fhir-to-csv-direct.ts:19-20
Timestamp: 2025-07-27T00:40:32.149Z
Learning: User leite08 acknowledged the duplicate inputBundle spread issue in packages/core/src/command/analytics-platform/fhir-to-csv/command/fhir-to-csv/fhir-to-csv-direct.ts and will fix it in a follow-up PR rather than in the current patch release PR #4255.

Learnt from: leite08
PR: metriport/metriport#4013
File: packages/core/src/fhir-to-cda/cda-templates/components/procedures.ts:104-105
Timestamp: 2025-06-12T22:53:09.606Z
Learning: User leite08 prefers responses in English only.

Learnt from: leite08
PR: metriport/metriport#3991
File: packages/core/src/external/aws/s3.ts:621-630
Timestamp: 2025-06-10T22:20:21.203Z
Learning: User leite08 prefers that all review comments are written in English.

Learnt from: RadmirGari
PR: metriport/metriport#4324
File: packages/core/src/domain/address.ts:7-7
Timestamp: 2025-08-13T16:08:03.688Z
Learning: In the Metriport codebase, packages/core legitimately depends on metriport/api-sdk and imports from it in multiple places. This is an established architectural pattern and is properly declared as a dependency in core's package.json.

Learnt from: thomasyopes
PR: metriport/metriport#4165
File: packages/data-transformation/fhir-to-csv/main.py:0-0
Timestamp: 2025-07-18T09:53:47.448Z
Learning: In packages/data-transformation/fhir-to-csv/main.py, the user thomasyopes prefers to let Snowflake operations (create_temp_tables, copy_into_temp_table, append_temp_tables, rename_temp_tables) hard fail rather than adding explicit try-catch error handling blocks.

Learnt from: thomasyopes
PR: metriport/metriport#4165
File: packages/data-transformation/fhir-to-csv/src/setupSnowflake/setupSnowflake.py:0-0
Timestamp: 2025-07-18T09:54:43.598Z
Learning: In packages/data-transformation/fhir-to-csv/src/setupSnowflake/setupSnowflake.py, the user thomasyopes prefers to log and swallow exceptions in the copy_into_temp_table function rather than re-raising them. This allows the data transformation job to continue processing other files even if one file fails to copy to Snowflake, supporting partial success scenarios in batch data processing.

Learnt from: thomasyopes
PR: metriport/metriport#4165
File: packages/data-transformation/fhir-to-csv/src/setupSnowflake/setupSnowflake.py:54-58
Timestamp: 2025-07-18T09:54:01.790Z
Learning: The user thomasyopes prefers to keep the setupSnowflake.py file in packages/data-transformation/fhir-to-csv/src/setupSnowflake/ as-is, declining suggestions for additional error handling around configuration file parsing.

Learnt from: thomasyopes
PR: metriport/metriport#4165
File: packages/data-transformation/fhir-to-csv/src/setupSnowflake/setupSnowflake.py:93-97
Timestamp: 2025-07-19T14:04:34.757Z
Learning: In packages/data-transformation/fhir-to-csv/src/setupSnowflake/setupSnowflake.py, the user thomasyopes accepts SQL injection risk in the append_job_tables function's DELETE query where patient_id is directly interpolated, based on their assessment that they control all inputs to the system.

Learnt from: thomasyopes
PR: metriport/metriport#4165
File: packages/data-transformation/fhir-to-csv/main.py:0-0
Timestamp: 2025-07-18T09:53:38.906Z
Learning: In packages/data-transformation/fhir-to-csv/main.py, S3 upload operations should be allowed to hard fail without try-catch blocks. The team prefers immediate failure over error handling and continuation for S3 upload operations in the FHIR-to-CSV transformation process.

Learnt from: thomasyopes
PR: metriport/metriport#4167
File: packages/infra/lib/analytics-platform/analytics-platform-stack.ts:56-64
Timestamp: 2025-07-09T20:41:08.549Z
Learning: In the analytics platform Snowflake integration (packages/infra/lib/analytics-platform/analytics-platform-stack.ts), the `integrationUserArn` field in the config temporarily contains an AWS account ID rather than an ARN. This is planned to be updated to an actual ARN in a second pass, at which point the IAM Role's assumedBy should be changed from AccountPrincipal to ArnPrincipal.

Learnt from: leite08
PR: metriport/metriport#3814
File: packages/api/src/routes/internal/medical/patient-consolidated.ts:141-174
Timestamp: 2025-05-20T21:26:26.804Z
Learning: The functionality introduced in packages/api/src/routes/internal/medical/patient-consolidated.ts is planned to be refactored in downstream PR #3857, including improvements to error handling and validation.

Learnt from: thomasyopes
PR: metriport/metriport#4165
File: packages/api/src/routes/internal/analytics-platform/index.ts:28-28
Timestamp: 2025-07-18T09:33:29.581Z
Learning: In packages/api/src/routes/internal/analytics-platform/index.ts, the patientId parameter handling is intentionally inconsistent: the `/fhir-to-csv` and `/fhir-to-csv/transform` endpoints require patientId (using getFromQueryOrFail) for single patient processing, while the `/fhir-to-csv/batch` endpoint treats patientId as optional (using getFromQuery) because it's designed to run over all patients when no specific patient ID is provided.

Learnt from: keshavsaharia
PR: metriport/metriport#4045
File: packages/core/src/external/surescripts/__tests__/file-names.test.ts:21-21
Timestamp: 2025-06-18T17:24:31.650Z
Learning: The team does all testing and deploying in PST local time, which helps avoid issues with date-dependent code in tests like buildRequestFileName that use dayjs().format('YYYYMMDD').

Learnt from: thomasyopes
PR: metriport/metriport#3427
File: packages/core/src/external/ehr/api/sync-patient.ts:16-55
Timestamp: 2025-03-11T20:42:46.516Z
Learning: In the patient synchronization architecture, the flow follows this pattern: (1) `ehr-sync-patient-cloud.ts` sends messages to an SQS queue, (2) the `ehr-sync-patient` Lambda consumes these messages, and (3) the Lambda uses the `syncPatient` function to make the API calls to process the patient data.

packages/data-transformation/fhir-to-csv/main.py (2)

71-76: Guard against missing "entry" in the bundle

Indexing with bundle["entry"] will raise if the key is absent. Using .get() avoids the KeyError and keeps the function idempotent.

Apply this diff:

-with open(local_bundle_key, "rb") as f:
-    bundle = json.load(f)
-    entries = bundle["entry"]
-    if entries is None or len(entries) < 1:
+with open(local_bundle_key, "rb") as f:
+    bundle = json.load(f)
+    entries = bundle.get("entry")
+    if not entries:
         print(f"Bundle {bundle_key} has no entries")
         return []

---

141-144: Do not call exit() inside a Lambda handler; return instead

Calling exit(0) in a Lambda can terminate the runtime process and degrade warm-start reuse. Return gracefully from the handler.

Apply this diff:

 if len(output_bucket_and_file_keys_and_table_names) < 1:
     print("No files were uploaded")
-    exit(0)
+    return

packages/utils/src/analytics-platform/1-fhir-to-csv.ts (1)

71-81: Include jobId in FIFO messageDeduplicationId to avoid cross-run collisions

Using only patientId for messageDeduplicationId can cause messages from successive runs to be dropped within SQS’s 5-minute dedup window. Including the per-run fhirToCsvJobId ensures deduplication is scoped to each job.

• File: packages/utils/src/analytics-platform/1-fhir-to-csv.ts
• Lines: 71–81

Apply this diff:

- await sqsClient.sendMessageToQueue(queueUrl, payload, {
-   fifo: true,
-   messageDeduplicationId: patientId,
-   messageGroupId: patientId,
- });
+ await sqsClient.sendMessageToQueue(queueUrl, payload, {
+   fifo: true,
+   messageDeduplicationId: `${fhirToCsvJobId}:${patientId}`,
+   messageGroupId: patientId,
+ });

Follow-up: if you require per-run ordering isolation instead of cross-run ordering by patient, you can also set

messageGroupId: `${fhirToCsvJobId}:${patientId}`,

packages/utils/src/fhir/bundle/find_with_custom_filter.sh (1)

97-110: Use jq’s exit code instead of string comparison; fix SC2155 (duplicate of earlier feedback).

Rely on jq -e’s exit status and avoid capturing output into a variable. This fixes ShellCheck SC2155 and removes brittle string comparisons.

Apply this diff:

 # Function to check if a file matches the custom filter
 check_file_with_filter() {
     local file="$1"
     local filter="$2"
     
-    # Apply the custom jq filter to the file
-    local result=$(jq -e "$filter" "$file" 2>/dev/null)
-    
-    if [ "$result" = "true" ]; then
-        return 0  # File matches the filter
-    fi
-    
-    return 1  # File doesn't match the filter
+    # Apply the custom jq filter; truthy -> exit 0, false/null -> non-zero
+    if jq -e "$filter" "$file" >/dev/null 2>&1; then
+        return 0
+    fi
+    return 1
 }

packages/utils/src/analytics-platform/util/check-merge-groups.ts (1)

249-255: Fix inconsistent script name in usage message.

Usage/help still references check-duplicate-keys-simple.ts while the file is check-merge-groups.ts.

Apply this diff:

     if (args.length === 0) {
-      console.log("Usage: ts-node check-duplicate-keys-simple.ts <folder-path>");
-      console.log("Example: ts-node check-duplicate-keys-simple.ts /path/to/your/folder");
+      console.log("Usage: ts-node check-merge-groups.ts <folder-path>");
+      console.log("Example: ts-node check-merge-groups.ts /path/to/your/folder");
       console.log("\nOr use current directory:");
-      console.log("ts-node check-duplicate-keys-simple.ts .");
+      console.log("ts-node check-merge-groups.ts .");
       process.exit(1);
     }

packages/core/src/external/aws/lambda.ts (1)

27-33: Use NodeHttpHandler for v3 client timeouts; httpOptions is not supported in AWS SDK v3

The v3 Lambda client ignores httpOptions. Configure timeouts via a custom NodeHttpHandler passed as requestHandler.

Apply this diff:

 export function makeLambdaClientV3(region: string, timeoutInMillis?: number): LambdaClientV3 {
-  const lambdaClient = new LambdaClientV3({
-    region,
-    ...(timeoutInMillis ? { httpOptions: { timeout: timeoutInMillis } } : {}),
-  });
-  return lambdaClient;
+  const requestHandler =
+    timeoutInMillis !== undefined
+      ? new NodeHttpHandler({
+          connectionTimeout: timeoutInMillis,
+          socketTimeout: timeoutInMillis,
+        })
+      : undefined;
+
+  return new LambdaClientV3({
+    region,
+    ...(requestHandler ? { requestHandler } : {}),
+  });
 }

Add this import near the top of the file:

import { NodeHttpHandler } from "@smithy/node-http-handler";

packages/utils/src/analytics-platform/2-merge-csvs.ts (3)

111-116: Make SQS deduplication ID deterministic per job and chunk.

Include cxId + mergeCsvJobId and sort IDs to avoid collisions and order-dependence during the FIFO 5-minute dedup window.

-        await sqsClient.sendMessageToQueue(queueUrl, payloadString, {
+        await sqsClient.sendMessageToQueue(queueUrl, payloadString, {
           fifo: true,
-          messageDeduplicationId: createUuidFromText(ptIdsOfThisRun.join(",")),
+          messageDeduplicationId: createUuidFromText(
+            JSON.stringify({ cxId, mergeCsvJobId, ids: [...ptIdsOfThisRun].sort() })
+          ),
           messageGroupId,
         });

---

39-41: Add validation for required CLI argument (fhirToCsvJobId).

Without this, downstream code may run with an undefined job ID and produce confusing failures.

 const fhirToCsvJobId = process.argv[2];
+if (!fhirToCsvJobId) {
+  console.error("ERROR: Missing required argument: fhirToCsvJobId");
+  console.error("Usage: ts-node src/analytics-platform/2-merge-csvs.ts <fhirToCsvJobId>");
+  process.exit(1);
+}

---

152-167: Fail fast if no patient IDs are found for the provided job ID.

Avoid silently proceeding with an empty set; this is likely a misconfiguration or wrong prefix.

 async function getPatientIdsFromFhirToCsvJob({
   fhirToCsvJobId,
 }: {
   fhirToCsvJobId: string;
 }): Promise<string[]> {
   const basePrefix = `${defaultPayload.sourcePrefix}/${fhirToCsvJobId}/`;
   const files = await s3Utils.listFirstLevelSubdirectories({
     bucket: bucketName,
     prefix: basePrefix,
   });
+  if (files.length === 0) {
+    throw new Error(
+      `No patient data found for fhirToCsvJobId=${fhirToCsvJobId} at prefix=${basePrefix} (bucket=${bucketName})`
+    );
+  }
   const patientIds = files.flatMap(file => {
     const item = file.Prefix?.endsWith("/") ? file.Prefix.slice(0, -1) : file.Prefix;
     return item?.split("/").pop() ?? [];
   });
-  return patientIds;
+  const validPatientIds = patientIds.filter(id => id && id.trim().length > 0);
+  if (validPatientIds.length === 0) {
+    throw new Error(
+      `No valid patient IDs extracted for fhirToCsvJobId=${fhirToCsvJobId} from prefix=${basePrefix}`
+    );
+  }
+  return validPatientIds;
 }

packages/utils/src/analytics-platform/3.readme.md (1)

1-1: Polish grammar and clarity (comma splice, hyphenation, and wording).

Suggest rephrasing to avoid the comma splice, use “third” instead of “3rd,” prefer “subfolder,” and clarify “starting with ‘3-’” as a “prefix”.

Apply this diff:

-The 3rd step is platform-specific, check the respective sub-folder for a script starting with "3-".
+The third step is platform-specific; check the respective subfolder for a script whose name starts with the prefix "3-".
+Example (Snowflake): packages/utils/src/analytics-platform/snowflake/3-ingest-from-merged-csvs.ts

packages/utils/src/fhir/bundle/find_with_custom_filter.sh (3)

81-86: Normalize DIRECTORY to avoid trailing-slash edge cases in relative path computation.

If the provided directory ends with a trailing slash, ${file#$DIRECTORY/} won’t strip the prefix as intended. Normalize once after validating the directory.

Apply this diff:

 if [ ! -d "$DIRECTORY" ]; then
     echo "Error: Directory '$DIRECTORY' does not exist"
     exit 1
 fi
+
+# Normalize DIRECTORY: remove trailing slash for consistent prefix stripping
+DIRECTORY="${DIRECTORY%/}"

---

17-18: Update jq homepage URL.

The jq project’s canonical site is now under jqlang.

-#   - jq must be installed (https://stedolan.github.io/jq/)
+#   - jq must be installed (https://jqlang.github.io/jq/)

---

1-3: Optional: Enable safer Bash mode early.

set -euo pipefail helps catch unset vars and early failures; setting IFS minimizes accidental word-splitting.

 #!/bin/bash
-
+# Safer bash defaults
+set -euo pipefail
+IFS=$'\n\t'

packages/utils/src/analytics-platform/util/check-merge-groups.ts (2)

60-61: Avoid passing objects directly to console logging; keep stack traces; comply with logging guidelines.

Don’t send objects as a second param to logs; format to string and include stack to retain context.

Apply this diff:

   } catch (error) {
-      console.error(`Error reading directory ${this.targetFolder}:`, error);
+      const msg = error instanceof Error ? error.stack ?? error.message : String(error);
+      console.error(`Error reading directory ${this.targetFolder}: ${msg}`);
       return [];
     }

     } catch (error) {
-      console.error(`Error processing ${filePath}:`, error);
+      const msg = error instanceof Error ? error.stack ?? error.message : String(error);
+      console.error(`Error processing ${filePath}: ${msg}`);
     }

Also applies to: 85-87

---

229-231: Deterministic processing and guideline-friendly loops.

For reproducible outputs and to avoid inline arrow callbacks (per guidelines), sort the files and use a for-of loop.

Apply this diff:

-    // Process each file
-    groupFiles.forEach(file => this.processFile(file));
+    // Process each file (sorted for deterministic output)
+    for (const file of groupFiles.sort()) {
+      this.processFile(file);
+    }

packages/core/src/external/aws/lambda-logic/README.md (1)

3-7: Tighten grammar and prepositions in the deprecation note

Small copy edits improve clarity and fix minor grammar issues (comma splice, preposition choice).

Apply this diff:

-This folder should not exist. If we put all "lambda logic" in this folder, we will have a hard time
-maintaining it, all lambdas' logic will be in a single place.
+This folder should not exist. If we put all "lambda logic" in this folder, we will have a hard time
+maintaining it, since all lambdas' logic would be concentrated in a single place.

-What we've been doing for all other lambdas, is to call commands/functions that are stored on the
-proper folder for each use case/business logic.
+What we've been doing for all other lambdas is to call commands/functions that live in the
+proper folder for each use case or business logic.

packages/data-transformation/fhir-to-csv/main.py (2)

60-71: Avoid opening the file when using s3_client.download_file()

download_file takes a destination path and manages the file itself. The with open(..., "wb") wrapper is unnecessary and can be removed.

Apply this diff:

-with open(local_bundle_key, "wb") as f:
-    try:
-        print(f"Downloading bundle {bundle_key} from {input_bucket} to {local_bundle_key}")
-        s3_client.download_file(input_bucket, bundle_key, local_bundle_key)
-        print(f"Downloaded bundle {bundle_key} from {input_bucket} to {local_bundle_key}")
-    except s3_client.exceptions.ClientError as e:
-        if e.response['Error']['Code'] == '404':
-            print(f"Bundle {bundle_key} not found in input bucket {input_bucket}")
-            raise ValueError("Bundle not found") from e
-        else:
-            raise e
+try:
+    print(f"Downloading bundle {bundle_key} from {input_bucket} to {local_bundle_key}")
+    s3_client.download_file(input_bucket, bundle_key, local_bundle_key)
+    print(f"Downloaded bundle {bundle_key} from {input_bucket} to {local_bundle_key}")
+except s3_client.exceptions.ClientError as e:
+    if e.response['Error']['Code'] == '404':
+        print(f"Bundle {bundle_key} not found in input bucket {input_bucket}")
+        raise ValueError("Bundle not found") from e
+    else:
+        raise e

---

1-25: Note: Many Snowflake-related imports are now unused

Given ENG-813 plans to remove the old Snowflake ingestion path, it’s fine short-term, but these imports will be flagged by linters. Consider removing them once ENG-813 lands.

packages/utils/src/analytics-platform/1-fhir-to-csv.ts (2)

21-24: Doc nit: pluralize “CSV file”

Each patient conversion produces multiple CSV files (one per table/resource). Adjust docs accordingly.

Apply this diff:

- * It sends a message to SQS per patient, consumed by a Lambda function that generates the
- * CSV file.
+ * It sends a message to SQS per patient, consumed by a Lambda function that generates the
+ * CSV files.

---

98-103: File write path robustness

fs.writeFileSync(outputFile, ...) writes to the CWD. If the script is run from a directory without write permission, this fails. Consider writing to a temp or known output dir, or at least logging the absolute path.

packages/infra/lib/analytics-platform/analytics-platform-stack.ts (1)

241-243: Fix misleading comment: bucket being granted here is analytics, not medical documents

The code grants read/write on analyticsPlatformBucket, but the comment mentions the medical documents bucket.

Apply this diff:

-    // Grant read to medical document bucket set on the api-stack
-    ownProps.analyticsPlatformBucket.grantReadWrite(fhirToCsvTransformLambda);
+    // Grant read/write to the analytics platform bucket (Snowflake landing zone)
+    ownProps.analyticsPlatformBucket.grantReadWrite(fhirToCsvTransformLambda);

packages/utils/src/analytics-platform/2-merge-csvs.ts (11)

91-96: Avoid multi-line logs and fix label to reflect SQS operations.

Current log uses a multi-line string and mentions “Lambda invocations” while we’re putting SQS messages.

-  log(
-    `>>> Running it for ${uniquePatientIds.length} patients (${patientIdChunks.length} chunks of ` +
-      `${maxPatientCountPerLambda} patients each)...\n- mergeCsvJobId: ${mergeCsvJobId}\n- fhirToCsvJobId: ${fhirToCsvJobId}` +
-      `\n- maxPatientCountPerLambda: ${maxPatientCountPerLambda}\n- numberOfParallelLambdaInvocations: ${numberOfParallelPutSqsOperations}` +
-      `\n- maxUncompressedSizePerFileInMB: ${maxUncompressedSizePerFileInMB}`
-  );
+  log(
+    `>>> Running it for ${uniquePatientIds.length} patients (${patientIdChunks.length} chunks of ` +
+      `${maxPatientCountPerLambda} patients each)...`
+  );
+  log(`- mergeCsvJobId: ${mergeCsvJobId}`);
+  log(`- fhirToCsvJobId: ${fhirToCsvJobId}`);
+  log(`- maxPatientCountPerLambda: ${maxPatientCountPerLambda}`);
+  log(`- numberOfParallelPutSqsOperations: ${numberOfParallelPutSqsOperations}`);
+  log(`- maxUncompressedSizePerFileInMB: ${maxUncompressedSizePerFileInMB}`);

---

118-121: Fix log wording: it’s one message per chunk and index is off-by-one.

Prevents logs like “Put 0 messages on queue”.

-        log(
-          `>>> Put ${index} messages on queue (${amountOfPatientsProcessed}/${uniquePatientIds.length} patients)`
-        );
+        log(
+          `>>> Put message ${index + 1} on queue (${amountOfPatientsProcessed}/${uniquePatientIds.length} patients)`
+        );

---

104-129: Sanitize timestamp for filenames (Windows-safe).

File names with : are invalid on Windows. Sanitize the per-chunk failure filename.

-      const runTimestamp = buildDayjs().toISOString();
+      const runTimestamp = buildDayjs().toISOString();
+      const runTimestampForFileName = runTimestamp.replace(/[:.]/g, "-");
@@
-        const outputFile = `2-merge-csvs_failed-patient-ids_${mergeCsvJobId}_${runTimestamp}.txt`;
+        const outputFile = `2-merge-csvs_failed-patient-ids_${mergeCsvJobId}_${runTimestampForFileName}.txt`;

---

50-53: Extract jitter values to constants (avoid magic numbers).

Improves readability and configurability per guidelines.

 const confirmationTime = dayjs.duration(1, "seconds");
-const messageGroupId = "merge-csvs";
 const numberOfParallelPutSqsOperations = 20;
+const minJitterMillis = 20;
+const maxJitterMillis = 100;

Note: Message group advice in a separate comment.

---

133-136: Use the jitter constants instead of literals.

     {
       numberOfParallelExecutions: numberOfParallelPutSqsOperations,
-      minJitterMillis: 20,
-      maxJitterMillis: 100,
+      minJitterMillis,
+      maxJitterMillis,
     }

---

87-89: Sort patient IDs to make chunking deterministic.

This ensures reproducible chunk composition across runs and environments.

-  const uniquePatientIds = [...new Set(patientsToMerge)];
+  const uniquePatientIds = [...new Set(patientsToMerge)].sort();

---

51-51: Confirm FIFO MessageGroupId strategy (single group serializes all jobs).

Using a static messageGroupId = "merge-csvs" will serialize all messages on the queue, across all CXs and jobs, potentially creating head-of-line blocking. If you want ordering only within a single run, consider a per-job group ID (e.g., merge-csvs:${cxId}:${mergeCsvJobId}) and define it after cxId/mergeCsvJobId are initialized.

If you choose to change it, move the declaration below the env variables and set:

const messageGroupId = `merge-csvs:${cxId}:${mergeCsvJobId}`;

---

184-185: Add top-level error handling for unhandled rejections.

Prevents silent failures and exits with a non-zero code for CI or scripting.

-main();
+main().catch(err => {
+  const { log } = out("");
+  log(`>>> FAILED: ${errorToString(err)}`);
+  process.exit(1);
+});

---

71-72: Remove unnecessary 100ms sleep at start of main.

No dependency requires this delay; it only slows execution.

-  await sleep(100);

---

127-145: Patient ID failure files: ensure they’re ignored and handled securely.

These files contain identifiers; ensure they’re .gitignored and stored in a secure location when running in shared environments.

Would you like me to add a .gitignore entry like:

2-merge-csvs_failed-patient-ids_*.txt

and/or make the output directory configurable via env?

---

15-15: Avoid cross-package relative import of Lambda types (tight coupling).

Importing types from packages/lambdas ties utils to an app package and breaks layering. Move the shared payload type to a shared module (e.g., @metriport/core or @metriport/shared) and import from there.

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled
• Linear integration is disabled

You can enable these sources in your CodeRabbit configuration.

Learnt from: leite08
PR: metriport/metriport#4255
File: packages/core/src/command/analytics-platform/fhir-to-csv/command/fhir-to-csv/fhir-to-csv-direct.ts:19-20
Timestamp: 2025-07-27T00:40:32.149Z
Learning: User leite08 acknowledged the duplicate inputBundle spread issue in packages/core/src/command/analytics-platform/fhir-to-csv/command/fhir-to-csv/fhir-to-csv-direct.ts and will fix it in a follow-up PR rather than in the current patch release PR #4255.

Learnt from: leite08
PR: metriport/metriport#4013
File: packages/core/src/fhir-to-cda/cda-templates/components/procedures.ts:104-105
Timestamp: 2025-06-12T22:53:09.606Z
Learning: User leite08 prefers responses in English only.

Learnt from: leite08
PR: metriport/metriport#3991
File: packages/core/src/external/aws/s3.ts:621-630
Timestamp: 2025-06-10T22:20:21.203Z
Learning: User leite08 prefers that all review comments are written in English.

Learnt from: thomasyopes
PR: metriport/metriport#4165
File: packages/data-transformation/fhir-to-csv/main.py:0-0
Timestamp: 2025-07-18T09:53:47.448Z
Learning: In packages/data-transformation/fhir-to-csv/main.py, the user thomasyopes prefers to let Snowflake operations (create_temp_tables, copy_into_temp_table, append_temp_tables, rename_temp_tables) hard fail rather than adding explicit try-catch error handling blocks.

Learnt from: thomasyopes
PR: metriport/metriport#4165
File: packages/data-transformation/fhir-to-csv/main.py:0-0
Timestamp: 2025-07-18T09:53:38.906Z
Learning: In packages/data-transformation/fhir-to-csv/main.py, S3 upload operations should be allowed to hard fail without try-catch blocks. The team prefers immediate failure over error handling and continuation for S3 upload operations in the FHIR-to-CSV transformation process.

Learnt from: thomasyopes
PR: metriport/metriport#4165
File: packages/data-transformation/fhir-to-csv/src/setupSnowflake/setupSnowflake.py:54-58
Timestamp: 2025-07-18T09:54:01.790Z
Learning: The user thomasyopes prefers to keep the setupSnowflake.py file in packages/data-transformation/fhir-to-csv/src/setupSnowflake/ as-is, declining suggestions for additional error handling around configuration file parsing.

Learnt from: thomasyopes
PR: metriport/metriport#4165
File: packages/data-transformation/fhir-to-csv/src/setupSnowflake/setupSnowflake.py:93-97
Timestamp: 2025-07-19T14:04:34.757Z
Learning: In packages/data-transformation/fhir-to-csv/src/setupSnowflake/setupSnowflake.py, the user thomasyopes accepts SQL injection risk in the append_job_tables function's DELETE query where patient_id is directly interpolated, based on their assessment that they control all inputs to the system.

Learnt from: thomasyopes
PR: metriport/metriport#4165
File: packages/data-transformation/fhir-to-csv/src/setupSnowflake/setupSnowflake.py:0-0
Timestamp: 2025-07-18T09:54:43.598Z
Learning: In packages/data-transformation/fhir-to-csv/src/setupSnowflake/setupSnowflake.py, the user thomasyopes prefers to log and swallow exceptions in the copy_into_temp_table function rather than re-raising them. This allows the data transformation job to continue processing other files even if one file fails to copy to Snowflake, supporting partial success scenarios in batch data processing.

Learnt from: leite08
PR: metriport/metriport#3814
File: packages/api/src/routes/internal/medical/patient-consolidated.ts:141-174
Timestamp: 2025-05-20T21:26:26.804Z
Learning: The functionality introduced in packages/api/src/routes/internal/medical/patient-consolidated.ts is planned to be refactored in downstream PR #3857, including improvements to error handling and validation.

Learnt from: leite08
PR: metriport/metriport#4255
File: packages/core/src/command/analytics-platform/fhir-to-csv/command/fhir-to-csv/fhir-to-csv-direct.ts:19-20
Timestamp: 2025-07-27T00:40:32.149Z
Learning: User leite08 acknowledged the duplicate inputBundle spread issue in packages/core/src/command/analytics-platform/fhir-to-csv/command/fhir-to-csv/fhir-to-csv-direct.ts and will fix it in a follow-up PR rather than in the current patch release PR #4255.

Learnt from: RamilGaripov
PR: metriport/metriport#3962
File: packages/api/src/sequelize/migrations/2025-06-17_00_create-cohort-tables.ts:50-56
Timestamp: 2025-06-17T20:03:18.931Z
Learning: In the Metriport codebase, the patient table ID is defined as DataTypes.STRING in the database schema, not UUID. Foreign key references to patient.id should use DataTypes.STRING to maintain consistency.

Learnt from: RamilGaripov
PR: metriport/metriport#3676
File: packages/core/src/command/hl7v2-subscriptions/hl7v2-to-fhir-conversion/shared.ts:1-10
Timestamp: 2025-04-16T00:25:25.196Z
Learning: The circular dependency between shared.ts (importing getPatientIdsOrFail) and adt/utils.ts (using unpackPidFieldOrFail) will be addressed in a follow-up PR.

Learnt from: thomasyopes
PR: metriport/metriport#4165
File: packages/api/src/routes/internal/analytics-platform/index.ts:28-28
Timestamp: 2025-07-18T09:33:29.581Z
Learning: In packages/api/src/routes/internal/analytics-platform/index.ts, the patientId parameter handling is intentionally inconsistent: the `/fhir-to-csv` and `/fhir-to-csv/transform` endpoints require patientId (using getFromQueryOrFail) for single patient processing, while the `/fhir-to-csv/batch` endpoint treats patientId as optional (using getFromQuery) because it's designed to run over all patients when no specific patient ID is provided.

Learnt from: leite08
PR: metriport/metriport#4194
File: packages/api/src/command/medical/patient/get-patient-facilities.ts:36-44
Timestamp: 2025-07-16T01:38:14.080Z
Learning: In packages/api/src/command/medical/patient/get-patient-facilities.ts, the patient.facilityIds field is strongly typed as array of strings (non-nullable), so null/undefined checks are not needed when accessing this property.

Learnt from: keshavsaharia
PR: metriport/metriport#3885
File: packages/utils/src/surescripts/generate-patient-load-file.ts:92-94
Timestamp: 2025-05-30T01:11:35.300Z
Learning: In V0 of the Surescripts implementation (packages/utils/src/surescripts/generate-patient-load-file.ts), there is always only 1 facility ID, so the current facility_ids parsing logic with substring operations is sufficient and additional validation is not necessary.

Learnt from: thomasyopes
PR: metriport/metriport#3427
File: packages/core/src/external/ehr/api/sync-patient.ts:16-55
Timestamp: 2025-03-11T20:42:46.516Z
Learning: In the patient synchronization architecture, the flow follows this pattern: (1) `ehr-sync-patient-cloud.ts` sends messages to an SQS queue, (2) the `ehr-sync-patient` Lambda consumes these messages, and (3) the Lambda uses the `syncPatient` function to make the API calls to process the patient data.

Learnt from: thomasyopes
PR: metriport/metriport#4167
File: packages/infra/lib/analytics-platform/analytics-platform-stack.ts:56-64
Timestamp: 2025-07-09T20:41:08.549Z
Learning: In the analytics platform Snowflake integration (packages/infra/lib/analytics-platform/analytics-platform-stack.ts), the `integrationUserArn` field in the config temporarily contains an AWS account ID rather than an ARN. This is planned to be updated to an actual ARN in a second pass, at which point the IAM Role's assumedBy should be changed from AccountPrincipal to ArnPrincipal.

packages/core/src/command/analytics-platform/fhir-to-csv/file-name.ts (1)

23-27: Table name parsing relies on fragile string manipulation.

The parsing function uses hard-coded array indices and assumes a specific file structure. This could fail silently if the key format changes.

This comment was already raised in the past review comments section. The parsing logic is brittle and would benefit from more robust validation and error handling.

packages/utils/src/analytics-platform/1-fhir-to-csv.ts (4)

1-1: Consider consolidating the header comment with existing structure.

The standalone header comment on Line 1 is redundant since the filename is already clear from the file path. Consider removing it to maintain consistency with other utility files.

-// 1-fhir-to-csv.ts
-

---

35-35: Increased concurrency may need monitoring.

The increase from 20 to 30 parallel executions could improve throughput but may also increase the risk of hitting SQS rate limits or Lambda concurrency constraints.

Consider making this configurable via environment variables to allow runtime adjustment based on observed performance and error rates.

-const numberOfParallelExecutions = 30;
+const numberOfParallelExecutions = parseInt(getEnvVar("PARALLEL_EXECUTIONS") ?? "30", 10);

---

77-77: TODO comment should be addressed or tracked.

The TODO comment about using FhirToCsvCloud suggests there may be a more appropriate abstraction available.

Would you like me to help investigate the FhirToCsvCloud abstraction or create an issue to track this potential refactoring?

---

96-96: Reduced jitter may require monitoring.

The decrease in maxJitterMillis from 200 to 100 could lead to more synchronized requests and potential thundering herd effects.

Consider monitoring error rates and adjusting jitter values if rate limiting issues occur.

packages/core/src/command/analytics-platform/merge-csvs/index.ts (2)

19-22: Consider making concurrency limits configurable.

The hard-coded concurrency limits could benefit from being environment-configurable for different deployment scenarios.

As mentioned in a past review comment, these values could be moved to environment variables via CDK for runtime adjustment.

const numberOfParallelListObjectsFromS3 = parseInt(process.env.PARALLEL_LIST_OBJECTS ?? "20", 10);
const numberOfParallelMergeFileGroups = parseInt(process.env.PARALLEL_MERGE_GROUPS ?? "5", 10);

---

409-438: Stream handling could be more robust.

The current implementation pipes S3 streams to gzip but doesn't handle potential stream errors during piping. Consider adding error handling to the pipe operation.

// Pipe S3 stream directly to gzip without loading into memory
-s3ReadStream.pipe(gzip, { end: false });
+s3ReadStream.pipe(gzip, { end: false });
+s3ReadStream.on("error", (error) => {
+  log(`Error reading file ${file.key}: ${errorToString(error)}`);
+  throw error;
+});

packages/lambdas/src/analytics-platform/merge-csvs.ts (3)

19-22: Remove unused exported type alias (or actually use it)

GroupAndMergeCSVsParamsLambda is not used. Either remove it or use it to type the parsed payload for stronger connections between schema and types.

Apply this diff if you choose to remove it:

-export type GroupAndMergeCSVsParamsLambda = Omit<
-  GroupAndMergeCSVsParams,
-  "sourceBucket" | "destinationBucket" | "region"
->;

---

24-46: Prefer a named handler (guideline) and avoid sending full event/patient IDs to Sentry/logs

• Style: avoid arrow functions for handlers (guideline), use a named function and wrap it.
• Telemetry hygiene: avoid capture.setExtra({ event }) and logging the entire payload; it can be large and may include patient IDs. Send only scoped metadata (job IDs, counts) and summarize logs.

Apply this diff:

-export const handler = capture.wrapHandler(async (event: SQSEvent) => {
-  capture.setExtra({ event, context: lambdaName });
-  const message = getSingleMessageOrFail(event.Records, lambdaName);
-  if (!message) return;
-  const parsedBody = parseBody(inputSchema, message.body);
-  capture.setExtra(parsedBody);
-  const { cxId, fhirToCsvJobId, mergeCsvJobId, patientIds, targetGroupSizeMB } = parsedBody;
-  const log = prefixedLog(`mergeCsvJobId ${mergeCsvJobId}`);
-  log(`Running with params:: ${JSON.stringify(parsedBody)}`);
-
-  const results = await groupAndMergeCSVs({
-    cxId,
-    fhirToCsvJobId,
-    mergeCsvJobId,
-    patientIds,
-    targetGroupSizeMB,
-    sourceBucket: bucket,
-    destinationBucket: bucket,
-    region,
-  });
-
-  return results;
-});
+async function rawHandler(event: SQSEvent) {
+  const message = getSingleMessageOrFail(event.Records, lambdaName);
+  if (!message) return;
+
+  const parsedBody = parseBody(inputSchema, message.body);
+  const { cxId, fhirToCsvJobId, mergeCsvJobId, patientIds, targetGroupSizeMB } = parsedBody;
+
+  // Keep Sentry extras minimal and PI/PHI-safe
+  capture.setExtra({
+    lambdaName,
+    messageId: message.messageId,
+    cxId,
+    fhirToCsvJobId,
+    mergeCsvJobId,
+    patientCount: patientIds.length,
+    targetGroupSizeMB,
+  });
+
+  const log = prefixedLog(`mergeCsvJobId ${mergeCsvJobId}`);
+  log(
+    `Running with cxId=${cxId}, fhirToCsvJobId=${fhirToCsvJobId}, mergeCsvJobId=${mergeCsvJobId}, ` +
+      `patientCount=${patientIds.length}, targetGroupSizeMB=${targetGroupSizeMB}`
+  );
+
+  const results = await groupAndMergeCSVs({
+    cxId,
+    fhirToCsvJobId,
+    mergeCsvJobId,
+    patientIds,
+    targetGroupSizeMB,
+    sourceBucket: bucket,
+    destinationBucket: bucket,
+    region,
+  });
+
+  return results;
+}
+
+export const handler = capture.wrapHandler(rawHandler);

---

48-54: Harden input validation: require ≥1 patient and a positive target size

Add simple constraints to prevent empty work and invalid sizes. Optionally cap to 1,200 (as per grouping step) for defense in depth.

Apply this diff:

 export const inputSchema = z.object({
   cxId: z.string(),
   fhirToCsvJobId: z.string(),
   mergeCsvJobId: z.string(),
-  patientIds: z.array(z.string()),
-  targetGroupSizeMB: z.number(),
+  patientIds: z.array(z.string()).min(1, "At least one patientId is required"),
+  targetGroupSizeMB: z.number().positive(),
 });

If you want to enforce the 1,200 cap here too:

-  patientIds: z.array(z.string()).min(1, "At least one patientId is required"),
+  patientIds: z.array(z.string()).min(1).max(1200),

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled
• Linear integration is disabled

You can enable these sources in your CodeRabbit configuration.

Learnt from: leite08
PR: metriport/metriport#4255
File: packages/core/src/command/analytics-platform/fhir-to-csv/command/fhir-to-csv/fhir-to-csv-direct.ts:19-20
Timestamp: 2025-07-27T00:40:32.149Z
Learning: User leite08 acknowledged the duplicate inputBundle spread issue in packages/core/src/command/analytics-platform/fhir-to-csv/command/fhir-to-csv/fhir-to-csv-direct.ts and will fix it in a follow-up PR rather than in the current patch release PR #4255.

Learnt from: leite08
PR: metriport/metriport#4013
File: packages/core/src/fhir-to-cda/cda-templates/components/procedures.ts:104-105
Timestamp: 2025-06-12T22:53:09.606Z
Learning: User leite08 prefers responses in English only.

Learnt from: leite08
PR: metriport/metriport#3991
File: packages/core/src/external/aws/s3.ts:621-630
Timestamp: 2025-06-10T22:20:21.203Z
Learning: User leite08 prefers that all review comments are written in English.

Learnt from: keshavsaharia
PR: metriport/metriport#3885
File: packages/core/src/external/sftp/surescripts/client.ts:1-134
Timestamp: 2025-05-28T19:23:20.179Z
Learning: In packages/core/src/external/sftp/surescripts/client.ts, the standalone getPatientLoadFileName function intentionally omits the transmission ID from the filename, which differs from the class method getPatientLoadFileName. This difference in filename generation is expected behavior.

Learnt from: leite08
PR: metriport/metriport#3814
File: packages/api/src/routes/internal/medical/patient-consolidated.ts:141-174
Timestamp: 2025-05-20T21:26:26.804Z
Learning: The functionality introduced in packages/api/src/routes/internal/medical/patient-consolidated.ts is planned to be refactored in downstream PR #3857, including improvements to error handling and validation.

Learnt from: thomasyopes
PR: metriport/metriport#4165
File: packages/api/src/routes/internal/analytics-platform/index.ts:28-28
Timestamp: 2025-07-18T09:33:29.581Z
Learning: In packages/api/src/routes/internal/analytics-platform/index.ts, the patientId parameter handling is intentionally inconsistent: the `/fhir-to-csv` and `/fhir-to-csv/transform` endpoints require patientId (using getFromQueryOrFail) for single patient processing, while the `/fhir-to-csv/batch` endpoint treats patientId as optional (using getFromQuery) because it's designed to run over all patients when no specific patient ID is provided.

Learnt from: thomasyopes
PR: metriport/metriport#3427
File: packages/core/src/external/ehr/api/sync-patient.ts:16-55
Timestamp: 2025-03-11T20:42:46.516Z
Learning: In the patient synchronization architecture, the flow follows this pattern: (1) `ehr-sync-patient-cloud.ts` sends messages to an SQS queue, (2) the `ehr-sync-patient` Lambda consumes these messages, and (3) the Lambda uses the `syncPatient` function to make the API calls to process the patient data.

Learnt from: leite08
PR: metriport/metriport#4350
File: packages/core/src/command/analytics-platform/merge-csvs/index.ts:336-363
Timestamp: 2025-08-15T03:55:25.167Z
Learning: For bulk upload operations (non-transactional flows), logging error details is sufficient and detailed error aggregation in thrown exceptions is not necessary, unlike transactional flows where detailed error information in exceptions may be more important.

Learnt from: leite08
PR: metriport/metriport#3412
File: packages/core/src/command/patient-import/steps/result/patient-import-result-cloud.ts:16-18
Timestamp: 2025-04-24T02:02:14.646Z
Learning: When logging payloads, it's acceptable to include them in logs if they only contain non-sensitive identifiers (like IDs) and are small enough to not create multi-line logs.

Learnt from: leite08
PR: metriport/metriport#4255
File: packages/core/src/command/analytics-platform/fhir-to-csv/command/fhir-to-csv/fhir-to-csv-direct.ts:19-20
Timestamp: 2025-07-27T00:40:32.149Z
Learning: User leite08 acknowledged the duplicate inputBundle spread issue in packages/core/src/command/analytics-platform/fhir-to-csv/command/fhir-to-csv/fhir-to-csv-direct.ts and will fix it in a follow-up PR rather than in the current patch release PR #4255.

packages/core/src/external/aws/lambda.ts (3)

1-1: Use NodeHttpHandler in AWS SDK v3; httpOptions is ignored in v3

httpOptions isn’t a valid config for v3 clients and will be silently ignored. Provide a custom requestHandler with NodeHttpHandler to enforce timeouts.

Apply this diff:

 import { InvokeCommandOutput, LambdaClient as LambdaClientV3 } from "@aws-sdk/client-lambda";
+import { NodeHttpHandler } from "@smithy/node-http-handler";
@@
-export function makeLambdaClientV3(region: string, timeoutInMillis?: number): LambdaClientV3 {
-  const lambdaClient = new LambdaClientV3({
-    region,
-    ...(timeoutInMillis ? { httpOptions: { timeout: timeoutInMillis } } : {}),
-  });
-  return lambdaClient;
-}
+export function makeLambdaClientV3(region: string, timeoutInMillis?: number): LambdaClientV3 {
+  const requestHandler = timeoutInMillis
+    ? new NodeHttpHandler({
+        connectionTimeout: timeoutInMillis,
+        socketTimeout: timeoutInMillis,
+      })
+    : undefined;
+
+  return new LambdaClientV3({
+    region,
+    ...(requestHandler ? { requestHandler } : {}),
+  });
+}

Also applies to: 27-33

---

97-106: Decode v3 Payload (Uint8Array) before JSON.parse

toString() on a Uint8Array won’t decode UTF-8 and can yield incorrect strings. Decode the bytes first.

 export function getLambdaErrorV3(result: InvokeCommandOutput): LambdaError | undefined {
   if (!result.Payload) return undefined;
   if (!isLambdaErrorV3(result)) return undefined;
-  const response = JSON.parse(result.Payload.toString());
+  const payloadStr = Buffer.from(result.Payload as Uint8Array).toString("utf-8");
+  const response = JSON.parse(payloadStr);
   return {
     errorType: response.errorType,
     errorMessage: response.errorMessage,
     log: logResultToString(result.LogResult),
   };
 }

---

223-250: Return decoded v3 payload string; avoid .toString() on Uint8Array

Ensure UTF-8 decoding before returning.

   if (isLambdaErrorV3(result)) {
@@
     throw new MetriportError(msg, undefined, { lambdaName, errorDetails });
   }
-  return result.Payload.toString();
+  return Buffer.from(result.Payload as Uint8Array).toString("utf-8");
 }

Optional cleanup (centralize decoding to avoid duplication with getLambdaErrorV3):

// helper (place near other utils in this file)
function decodePayload(payload: Uint8Array | string): string {
  return typeof payload === "string" ? payload : Buffer.from(payload).toString("utf-8");
}

// usage:
const payloadStr = decodePayload(result.Payload as Uint8Array | string);

packages/utils/src/analytics-platform/util/check-merge-groups.ts (3)

249-254: Fix inconsistent script name in usage message

The usage text still references check-duplicate-keys-simple.ts; update to check-merge-groups.ts.

-      console.log("Usage: ts-node check-duplicate-keys-simple.ts <folder-path>");
-      console.log("Example: ts-node check-duplicate-keys-simple.ts /path/to/your/folder");
+      console.log("Usage: ts-node check-merge-groups.ts <folder-path>");
+      console.log("Example: ts-node check-merge-groups.ts /path/to/your/folder");
       console.log("\nOr use current directory:");
-      console.log("ts-node check-duplicate-keys-simple.ts .");
+      console.log("ts-node check-merge-groups.ts .");

---

93-103: Deduplicate per-key file list before counting duplicates (fix false positives)

Current logic uses raw occurrences (files.length), inflating counts when the same key repeats within a single groups file. Count unique files and use that for the duplicate condition and count.

   private findDuplicates(): void {
     this.keyMap.forEach((files, key) => {
-      if (files.length > 1) {
-        this.duplicateKeys.push({
-          key,
-          files: [...new Set(files)], // Remove duplicate file references
-          count: files.length,
-        });
-      }
+      const uniqueFiles = [...new Set(files)];
+      if (uniqueFiles.length > 1) {
+        this.duplicateKeys.push({
+          key,
+          files: uniqueFiles,
+          count: uniqueFiles.length,
+        });
+      }
     });
   }

---

190-194: Export unique-file counts in JSON (align with detection semantics)

allKeys[].count should reflect the number of unique group files per key, not raw occurrences.

-      allKeys: Array.from(this.keyMap.entries()).map(([key, files]) => ({
-        key,
-        files: [...new Set(files)],
-        count: files.length,
-      })),
+      allKeys: Array.from(this.keyMap.entries()).map(([key, files]) => {
+        const uniqueFiles = [...new Set(files)];
+        return {
+          key,
+          files: uniqueFiles,
+          count: uniqueFiles.length,
+        };
+      }),

packages/utils/src/fhir/bundle/find_with_custom_filter.sh (2)

97-110: Use jq’s exit code instead of string comparison; fix SC2155

This was previously suggested and acknowledged as a working utility. Keeping a record here since ShellCheck flags SC2155 and comparing literal “true” is brittle. If you prefer to leave as-is, feel free to ignore.

Apply this diff:

 # Function to check if a file matches the custom filter
 check_file_with_filter() {
     local file="$1"
     local filter="$2"
-    
-    # Apply the custom jq filter to the file
-    local result=$(jq -e "$filter" "$file" 2>/dev/null)
-    
-    if [ "$result" = "true" ]; then
-        return 0  # File matches the filter
-    fi
-    
-    return 1  # File doesn't match the filter
+    # Apply the custom jq filter; truthy -> exit 0, false/null -> non-zero
+    if jq -e "$filter" "$file" >/dev/null 2>&1; then
+        return 0
+    fi
+    return 1
 }

---

112-118: Make file discovery and iteration NUL-safe; avoid word-splitting issues

Prior feedback suggested switching to -print0 plus a NUL-delimited read loop to handle filenames with spaces/newlines. If you’re intentionally keeping it simple for a utility, you can skip; otherwise, this improves robustness.

Apply this diff:

-# Find all JSON files in the directory and subdirectories up to maxdepth
-json_files=$(find "$DIRECTORY" -maxdepth "$MAXDEPTH" -name "*.json" -type f | sort)
-
-if [ -z "$json_files" ]; then
-    echo "No JSON files found in directory: $DIRECTORY"
-    exit 0
-fi
+# Counter to detect if any JSON files exist
+scanned_count=0
@@
-# Process each JSON file
-for file in $json_files; do
+# Process each JSON file (NUL-delimited to be safe with spaces/newlines)
+while IFS= read -r -d '' file; do
+    scanned_count=$((scanned_count + 1))
@@
-done
+done < <(find "$DIRECTORY" -maxdepth "$MAXDEPTH" -type f -name "*.json" -print0 | sort -z)
+
+if [ "$scanned_count" -eq 0 ]; then
+    echo "No JSON files found in directory: $DIRECTORY"
+    exit 0
+fi

Also applies to: 129-147

packages/utils/src/analytics-platform/2-merge-csvs.ts (1)

85-96: Fail fast when no patient IDs are found for the job

This was previously suggested: continuing with an empty set can hide misconfigurations (wrong prefix) and waste time. Throw early with a descriptive message.

Apply this diff:

   const uniquePatientIds = [...new Set(patientsToMerge)];
   const patientIdChunks = chunk(uniquePatientIds, maxPatientCountPerLambda);
 
+  if (uniquePatientIds.length === 0) {
+    throw new Error(
+      `No patient IDs found for fhirToCsvJobId=${fhirToCsvJobId} ` +
+      `at prefix=${buildFhirToCsvJobPrefix({ cxId, jobId: fhirToCsvJobId })}`
+    );
+  }

packages/core/src/external/aws/lambda.ts (3)

66-70: Docs mismatch the function behavior

The JSDoc says “Checks whether the lambda invocation was successful”, but the function returns true when there is an error (FunctionError present). Update the wording to avoid confusion.

-/**
- * Checks whether the lambda invocation was successful.
- */
+/**
+ * Checks whether the lambda invocation returned an error (FunctionError is set).
+ */
 export function isLambdaErrorV3(result: InvokeCommandOutput): boolean {
   return result.FunctionError !== undefined;
 }

---

196-209: Tighten overload discriminant for failGracefully

Using boolean | false collapses to boolean, so the overload doesn’t properly discriminate return types. Use the literal type false in the first signature.

 export function getLambdaResultPayloadV3(params: {
   result: InvokeCommandOutput;
   lambdaName?: string;
-  failGracefully?: boolean | false;
+  failGracefully?: false;
   failOnEmptyResponse?: boolean | false;
   log?: typeof console.log;
 }): string;
 export function getLambdaResultPayloadV3(params: {
   result: InvokeCommandOutput;
   lambdaName?: string;
   failGracefully: true;
   failOnEmptyResponse?: boolean | false;
   log?: typeof console.log;
 }): string | undefined;

---

213-216: Use a distinct log prefix for the v3 path

Helps trace logs to the v3 helper.

 export function getLambdaResultPayloadV3({
   result,
   lambdaName = "<unknown-name>",
   failGracefully = false,
   failOnEmptyResponse = true,
-  log = out("getLambdaResultPayload").log,
+  log = out("getLambdaResultPayloadV3").log,

packages/utils/src/analytics-platform/3.readme.md (1)

1-1: Tighten grammar and clarity in the sentence.

Replace the hyphenated “sub-folder” and split into two sentences for readability.

Apply this diff:

-The 3rd step is platform-specific, check the respective sub-folder for a script starting with "3-".
+The 3rd step is platform-specific. Check the respective subfolder for a script starting with "3-".

packages/utils/src/analytics-platform/snowflake/3-ingest-from-merged-csvs.ts (5)

23-39: Fix typos and align the docstring with the actual behavior (no max-rows arg, no patientIds/includeDeletes).

Documentation mentions parameters/features not present in the script and has a typo (“buy” → “by”). Align with the current S3 layout and usage.

Apply this diff:

- * It relies on the mergeCsvJobId to be passed as a parameter. This ID is determined buy the 2nd step in
- * the flow, the script `2-merge-csvs.ts`.
+ * It relies on the mergeCsvJobId to be passed as a parameter. This ID is determined by the 2nd step
+ * in the flow (`2-merge-csvs.ts`).
  *
- * It relies on the merged CSV files to be on the configured bucket, with the following structure:
- *
- * ./snowflake/merged/cxId/patientId/jobId/_tmp_fhir-to-csv_output_cxId_patientId_resourceType.csv
- *
- * As a required runtime parameter, it needs the max number of rows to be used from each file found on S3.
- *
- * Set these constants based on the output from the merge-csvs lambda:
- * - patientIds
- * - mergeCsvJobId
- *
- * Set this constant to determine whether to include deletes while updating Snowflake, to strain
- * transaction control:
- * - includeDeletes
+ * It expects the merged CSV files to be in the configured S3 bucket under:
+ *   s3://<bucket>/${buildMergeCsvsJobPrefix({ cxId, jobId: mergeCsvJobId })}
+ * Files are organized in per-resource-type folders (e.g., .../observation/, .../patient/).

---

49-55: Prefer stderr + exit code for missing CLI arg (cleaner UX than throwing).

Throwing prints a stack trace which is noisy for a simple usage error. Emitting to stderr and exiting with code 1 is cleaner.

Apply this diff:

 const mergeCsvJobId = process.argv[2];
 if (!mergeCsvJobId) {
-  console.log(
-    "Usage: ts-node src/analytics-platform/snowflake/3-ingest-from-merged-csvs.ts <mergeCsvJobId>"
-  );
-  throw new Error("mergeCsvJobId is required");
+  console.error("Error: mergeCsvJobId is required");
+  console.error(
+    "Usage: ts-node src/analytics-platform/snowflake/3-ingest-from-merged-csvs.ts <mergeCsvJobId>"
+  );
+  process.exit(1);
 }

---

103-105: Short-circuit when no S3 files are found to avoid unnecessary Snowflake connection.

Minor optimization: skip connecting when there’s nothing to ingest.

Apply this diff:

     const files = await s3Utils.listObjects(bucketName, prefixName);
     console.log(`Found ${files.length} files in ${prefixUrl}`);
+    if (files.length === 0) {
+      console.log(`No files found under ${prefixUrl} - nothing to ingest`);
+      return;
+    }

---

156-159: Avoid false positives when checking for resource files (guard against prefix collisions).

Using includes(resourceType) can match superset names (e.g., condition vs condition_code_coding). Check for the folder delimiter to ensure we only process when the specific resource folder exists.

Apply this diff:

-  if (!files.some(file => file.Key?.includes(resourceType))) {
+  if (!files.some(file => file.Key && file.Key.includes(`/${resourceType}/`))) {
     console.log(`>>> No files found for ${resourceType} - skipping`);
     return;
   }

---

221-221: Quote column identifiers to avoid issues with reserved words or special characters.

INI keys are assumed to be valid unquoted identifiers. Quoting is safer and future-proof.

Apply this diff:

-    columnDefs[resourceType] = columns.map(column => `${column} VARCHAR`).join(", ");
+    columnDefs[resourceType] = columns
+      .map(col => `"${col.replace(/"/g, '""')}" VARCHAR`)
+      .join(", ");

packages/core/src/command/analytics-platform/merge-csvs/__tests__/index.test.ts (2)

1-1: Avoid file-wide ESLint disable for non-null assertions in tests

Instead of a global disable, guard the specific variables before use. This keeps the rule active and avoids masking future issues.

Apply a minimal refactor around the two non-null usages:

-/* eslint-disable @typescript-eslint/no-non-null-assertion */

For the large file assertion (around lines 102–107):

-      expect(largeFileGroup).toBeDefined();
-      expect(largeFileGroup!.totalSize).toBe(largeFileSize);
+      if (!largeFileGroup) {
+        throw new Error("Expected a group containing the large file");
+      }
+      expect(largeFileGroup.totalSize).toBe(largeFileSize);

For the “very large target” scenario (around lines 211–214):

-      expect(groups[0]!.files).toHaveLength(2);
+      const first = groups[0];
+      if (!first) throw new Error("Expected at least one group");
+      expect(first.files).toHaveLength(2);

---

7-13: Prefer using MB_IN_BYTES constant to avoid magic numbers (1024 * 1024)

Since MB_IN_BYTES is exported, using it improves readability and avoids repeated literals.

Example import adjustment:

-import { FileInfo, groupFilesByTypeAndSize } from "../index";
+import { MB_IN_BYTES, FileInfo, groupFilesByTypeAndSize } from "../index";

Example usage:

-{ key: "path/patient1/condition.csv", size: 50 * 1024 * 1024, tableName: "condition" }
+{ key: "path/patient1/condition.csv", size: 50 * MB_IN_BYTES, tableName: "condition" }

If you prefer minimal churn, this can be deferred.

Also applies to: 30-33, 46-51, 133-138, 151-156, 248-254, 265-270

packages/utils/src/analytics-platform/util/check-merge-groups.ts (2)

68-85: Avoid inflating counts when the same key appears multiple times in one groups.json

Even with downstream dedup, it’s cleaner to record each (key, groups-file) pair once. Deduplicate keys within a single file before updating the map.

   private processFile(filePath: string): void {
     try {
       console.log(`Processing: ${path.basename(filePath)}`);

       const content = fs.readFileSync(filePath, "utf-8");
       const data: GroupEntry[] = JSON.parse(content);

-      data.forEach(group => {
-        group.files.forEach(file => {
-          const key = file.key;
-          if (!this.keyMap.has(key)) {
-            this.keyMap.set(key, []);
-          }
-          // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
-          this.keyMap.get(key)!.push(path.basename(filePath));
-        });
-      });
+      const fileBase = path.basename(filePath);
+      const seenKeysInThisFile = new Set<string>();
+
+      data.forEach(group => {
+        group.files.forEach(file => {
+          const key = file.key;
+          if (seenKeysInThisFile.has(key)) return;
+          seenKeysInThisFile.add(key);
+
+          if (!this.keyMap.has(key)) {
+            this.keyMap.set(key, []);
+          }
+          // eslint-disable-next-line @typescript-eslint/no-non-null-assertion
+          this.keyMap.get(key)!.push(fileBase);
+        });
+      });
     } catch (error) {
       console.error(`Error processing ${filePath}:`, error);
     }
   }

---

108-133: Optional: Exit with non-zero code when duplicates are found (CI-friendly)

If you intend to wire this into CI, failing the script on duplicates improves automation. You can gate on this.duplicateKeys.length.

Example approach (after generateReport/exportResults):

   // Export results
   this.exportResults();
+
+  // Indicate failure via exit code when duplicates are found (optional)
+  if (this.duplicateKeys.length > 0) {
+    process.exitCode = 2;
+  }

If desired, I can provide a follow-up patch that adds a --fail-on-duplicates flag instead.

Also applies to: 176-200

packages/utils/src/fhir/bundle/find_with_custom_filter.sh (3)

27-44: Exit with code 0 on --help; keep “usage” reusable for error paths

Help should not signal an error. Make usage accept an exit code, defaulting to 1, and call it with 0 for -h/--help.

Apply this diff:

-show_usage() {
-    echo "Usage: $0 -d <directory> -f <jq_filter> [--maxdepth <depth>]"
+show_usage() {
+    local exit_code="${1:-1}"
+    echo "Usage: $0 -d <directory> -f <jq_filter> [--maxdepth <depth>]"
@@
-    echo "  $0 --directory . --filter '[.entry[] | select(.resource.resourceType == \"Encounter\")] | length > 0' --maxdepth 5"
-    exit 1
+    echo "  $0 --directory . --filter '[.entry[] | select(.resource.resourceType == \"Encounter\")] | length > 0' --maxdepth 5"
+    exit "$exit_code"
 }
@@
-        -h|--help)
-            show_usage
+        -h|--help)
+            show_usage 0

Also applies to: 66-67

---

1-2: Harden the script with strict mode

Enable strict-er bash options to fail fast on errors.

Apply this diff:

 #!/bin/bash
+
+set -euo pipefail

---

112-114: Ensure cross-platform compatibility for find on BSD/macOS

On macOS’s default /usr/bin/find, the -maxdepth flag isn’t supported. To keep this script portable, detect GNU find (installed as gfind via Homebrew) or fall back to a -prune-based approach.

• File needing update:
packages/utils/src/fhir/bundle/find_with_custom_filter.sh

• Replace the direct -maxdepth invocation with one of the following patterns:

1. Detect GNU find

+# Prefer GNU find if available
+if command -v gfind >/dev/null 2>&1; then
+  FIND_CMD=gfind
+else
+  FIND_CMD=find
+fi
 json_files=$(
-  find "$DIRECTORY" -maxdepth "$MAXDEPTH" -name "*.json" -type f \
+  $FIND_CMD "$DIRECTORY" -maxdepth "$MAXDEPTH" -name "*.json" -type f \
    | sort
 )

2. Portable -prune fallback (no -maxdepth)

json_files=$(
  find "$DIRECTORY" -type d \
    \( $(printf '%s -o ' "$(printf -- "-path %q/%s/*" "$DIRECTORY" "$(printf '/..%.0s' $(seq 1 $MAXDEPTH))")") -prune \) \
    -o -type f -name '*.json' -print \
  | sort
)

Choose the approach that best fits your environment and team’s requirements.

packages/utils/src/analytics-platform/2-merge-csvs.ts (2)

26-38: Minor grammar in script header

“invoke” → “invokes”, and a couple of phrasing tweaks.

Apply this diff:

- * This script invoke the lambda that merges the patients' CSVs files into single files.
+ * This script invokes the Lambda that merges patients’ CSV files into single files.
@@
- * It needs to be run after the lambdas that convert the FHIR data to CSV are done
+ * Run it after the Lambdas that convert FHIR data to CSV are done
@@
- * We need the output `fhirToCsvJobId` of that script as an argument.
+ * It requires the `fhirToCsvJobId` output from that script as an argument.

---

126-128: Clarify log message: index is a chunk index, not a message count

The current text reads as if multiple messages were put; it’s one message per chunk. Make the wording explicit and show progress.

Apply this diff:

-        log(
-          `>>> Put ${index} messages on queue (${amountOfPatientsProcessed}/${uniquePatientIds.length} patients)`
-        );
+        log(
+          `>>> Put message ${index + 1}/${patientIdChunks.length} on queue ` +
+          `(${amountOfPatientsProcessed}/${uniquePatientIds.length} patients)`
+        );

packages/core/src/command/analytics-platform/fhir-to-csv/file-name.ts (1)

23-27: Avoid magic indexes when parsing table name; use a defensive regex

Relying on split("/")[5] and slice(6) is brittle if file naming or path depth changes. A targeted regex is more maintainable and self-explanatory.

Apply this diff:

 export function parseTableNameFromFhirToCsvFileKey(key: string): string {
-  // e.g.: snowflake/fhir-to-csv/cx=cx-id/f2c=2025-08-08T02-18-56/pt=patient-id/_tmp_fhir-to-csv_output_cx-id_patient-id_condition.csv
-  const tableName = key.split("/")[5]?.split("_").slice(6).join("_").split(".")[0] || "";
-  return tableName;
+  // e.g.: .../pt=patient-id/_tmp_fhir-to-csv_output_<cxId>_<patientId>_<table>.csv
+  const match = key.match(/_tmp_fhir-to-csv_output_[^_]+_[^_]+_(?<table>[^/.]+)\.csv$/);
+  return match?.groups?.table ?? "";
 }

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

💡 Knowledge Base configuration:

• MCP integration is disabled by default for public repositories
• Jira integration is disabled
• Linear integration is disabled

You can enable these sources in your CodeRabbit configuration.

Learnt from: leite08
PR: metriport/metriport#4255
File: packages/core/src/command/analytics-platform/fhir-to-csv/command/fhir-to-csv/fhir-to-csv-direct.ts:19-20
Timestamp: 2025-07-27T00:40:32.149Z
Learning: User leite08 acknowledged the duplicate inputBundle spread issue in packages/core/src/command/analytics-platform/fhir-to-csv/command/fhir-to-csv/fhir-to-csv-direct.ts and will fix it in a follow-up PR rather than in the current patch release PR #4255.

Learnt from: leite08
PR: metriport/metriport#4013
File: packages/core/src/fhir-to-cda/cda-templates/components/procedures.ts:104-105
Timestamp: 2025-06-12T22:53:09.606Z
Learning: User leite08 prefers responses in English only.

Learnt from: leite08
PR: metriport/metriport#3991
File: packages/core/src/external/aws/s3.ts:621-630
Timestamp: 2025-06-10T22:20:21.203Z
Learning: User leite08 prefers that all review comments are written in English.

Learnt from: leite08
PR: metriport/metriport#3814
File: packages/api/src/routes/internal/medical/patient-consolidated.ts:141-174
Timestamp: 2025-05-20T21:26:26.804Z
Learning: The functionality introduced in packages/api/src/routes/internal/medical/patient-consolidated.ts is planned to be refactored in downstream PR #3857, including improvements to error handling and validation.

Learnt from: thomasyopes
PR: metriport/metriport#4165
File: packages/api/src/routes/internal/analytics-platform/index.ts:28-28
Timestamp: 2025-07-18T09:33:29.581Z
Learning: In packages/api/src/routes/internal/analytics-platform/index.ts, the patientId parameter handling is intentionally inconsistent: the `/fhir-to-csv` and `/fhir-to-csv/transform` endpoints require patientId (using getFromQueryOrFail) for single patient processing, while the `/fhir-to-csv/batch` endpoint treats patientId as optional (using getFromQuery) because it's designed to run over all patients when no specific patient ID is provided.

Learnt from: RamilGaripov
PR: metriport/metriport#3962
File: packages/api/src/sequelize/migrations/2025-06-17_00_create-cohort-tables.ts:50-56
Timestamp: 2025-06-17T20:03:18.931Z
Learning: In the Metriport codebase, the patient table ID is defined as DataTypes.STRING in the database schema, not UUID. Foreign key references to patient.id should use DataTypes.STRING to maintain consistency.

Learnt from: leite08
PR: metriport/metriport#4255
File: packages/core/src/command/analytics-platform/fhir-to-csv/command/fhir-to-csv/fhir-to-csv-direct.ts:19-20
Timestamp: 2025-07-27T00:40:32.149Z
Learning: User leite08 acknowledged the duplicate inputBundle spread issue in packages/core/src/command/analytics-platform/fhir-to-csv/command/fhir-to-csv/fhir-to-csv-direct.ts and will fix it in a follow-up PR rather than in the current patch release PR #4255.

Learnt from: RamilGaripov
PR: metriport/metriport#3676
File: packages/core/src/command/hl7v2-subscriptions/hl7v2-to-fhir-conversion/shared.ts:1-10
Timestamp: 2025-04-16T00:25:25.196Z
Learning: The circular dependency between shared.ts (importing getPatientIdsOrFail) and adt/utils.ts (using unpackPidFieldOrFail) will be addressed in a follow-up PR.

Learnt from: leite08
PR: metriport/metriport#4194
File: packages/api/src/command/medical/patient/get-patient-facilities.ts:36-44
Timestamp: 2025-07-16T01:38:14.080Z
Learning: In packages/api/src/command/medical/patient/get-patient-facilities.ts, the patient.facilityIds field is strongly typed as array of strings (non-nullable), so null/undefined checks are not needed when accessing this property.

Learnt from: keshavsaharia
PR: metriport/metriport#3885
File: packages/utils/src/surescripts/generate-patient-load-file.ts:92-94
Timestamp: 2025-05-30T01:11:35.300Z
Learning: In V0 of the Surescripts implementation (packages/utils/src/surescripts/generate-patient-load-file.ts), there is always only 1 facility ID, so the current facility_ids parsing logic with substring operations is sufficient and additional validation is not necessary.

Learnt from: leite08
PR: metriport/metriport#4350
File: packages/utils/src/analytics-platform/snowflake/3-ingest-from-merged-csvs.ts:159-161
Timestamp: 2025-08-15T03:59:27.025Z
Learning: User leite08 prefers fail-fast behavior for bulk import scripts in Snowflake. When tables already exist, the script should fail rather than continue, as this prevents accidental duplicate data ingestion. The `IF NOT EXISTS` clause should be omitted in bulk import scenarios to ensure data integrity.

Learnt from: leite08
PR: metriport/metriport#3611
File: packages/utils/src/carequality/download-directory.ts:68-82
Timestamp: 2025-04-05T16:02:22.473Z
Learning: Utility scripts in the packages/utils directory don't require comprehensive error handling like production code.

Learnt from: leite08
PR: metriport/metriport#3857
File: packages/utils/src/consolidated/filter-consolidated.ts:39-68
Timestamp: 2025-05-27T23:51:45.100Z
Learning: Utility scripts in the packages/utils directory don't require comprehensive error handling or logging changes (like switching from console.log to out().log), as they are meant for local/developer environments, not production code.

Learnt from: leite08
PR: metriport/metriport#3468
File: packages/utils/src/s3/list-objects.ts:17-28
Timestamp: 2025-03-14T00:30:18.860Z
Learning: Files under `./packages/utils` folder are utility scripts and don't require the same coding standards used in other packages (like error handling, structured logging, etc.).

Learnt from: leite08
PR: metriport/metriport#3412
File: packages/utils/src/patient-import/send-webhook.ts:41-55
Timestamp: 2025-04-24T02:17:02.661Z
Learning: Files under `packages/utils` don't need robust error handling as they are utility scripts meant for local/developer environments, not production code.

Learnt from: leite08
PR: metriport/metriport#3924
File: packages/utils/src/open-search/ingest-patients.ts:73-74
Timestamp: 2025-05-30T14:19:02.018Z
Learning: Utility scripts in the codebase don't need to strictly follow all coding guidelines, particularly around logging practices like avoiding multi-line logs or objects as second parameters.

Learnt from: leite08
PR: metriport/metriport#3468
File: packages/utils/src/s3/list-objects.ts:12-13
Timestamp: 2025-03-14T00:31:01.986Z
Learning: Files under the `./packages/utils` folder are utility scripts and don't require the same coding standards as other packages. They often contain placeholder values meant to be filled in by developers before use.

Learnt from: lucasdellabella
PR: metriport/metriport#3941
File: packages/utils/src/hl7v2-notifications/unpack-patient-roster-identifier.ts:61-64
Timestamp: 2025-05-31T23:09:05.757Z
Learning: Utility scripts in the utils package may have relaxed coding standards compared to production code, particularly for error handling. When reviewing debugging scripts or one-off utilities, apply less stringent requirements for error handling patterns.

Learnt from: RamilGaripov
PR: metriport/metriport#4259
File: packages/utils/src/document/reconversion-kickoff-loader.ts:78-81
Timestamp: 2025-07-27T23:42:34.014Z
Learning: For utility scripts in packages/utils, RamilGaripov prefers to keep them simple without extensive validation or error handling, as they are local developer tools where the developer controls the input and execution environment.

Learnt from: leite08
PR: metriport/metriport#3469
File: packages/utils/src/s3/list-objects.ts:23-27
Timestamp: 2025-03-14T01:13:34.887Z
Learning: Files under `./packages/utils` are permitted to use `console.log` directly and are not required to use `out().log` instead, as they're exempt from certain coding standards that apply to the rest of the codebase.

Learnt from: leite08
PR: metriport/metriport#3648
File: packages/utils/src/csv/find-dups.ts:14-14
Timestamp: 2025-04-10T16:01:21.135Z
Learning: Files under `packages/utils` are allowed to have empty strings for input parameters as these are meant to be manually modified by developers before running scripts in their local environments.

Learnt from: lucasdellabella
PR: metriport/metriport#4382
File: packages/utils/src/hl7v2-notifications/reprocess-adt-conversion-bundles/common.ts:36-48
Timestamp: 2025-08-15T18:57:28.999Z
Learning: In utility scripts like packages/utils/src/hl7v2-notifications/reprocess-adt-conversion-bundles/common.ts, user lucasdellabella prefers to let bundle parsing errors fail fast rather than adding error handling, as it provides immediate visibility into data issues during script execution.

Learnt from: leite08
PR: metriport/metriport#3611
File: packages/utils/src/carequality/download-directory.ts:33-66
Timestamp: 2025-04-05T16:02:31.219Z
Learning: In the Metriport codebase, utility scripts in the packages/utils directory have different error handling requirements compared to production code, and do not necessarily need the same level of error handling.

Learnt from: keshavsaharia
PR: metriport/metriport#4161
File: packages/utils/src/surescripts/compare/dr-first.ts:253-267
Timestamp: 2025-08-01T18:20:14.618Z
Learning: For one-off utility scripts in packages/utils/src/surescripts/, it's acceptable to ignore certain code quality issues like array modification during iteration, as these scripts prioritize functionality over perfect code patterns.

Learnt from: leite08
PR: metriport/metriport#3859
File: packages/utils/src/consolidated/seed-patient-consolidated.ts:94-94
Timestamp: 2025-05-26T16:24:13.656Z
Learning: For utility scripts in the packages/utils directory, leite08 accepts simpler string replacement approaches (like replaceAll) even when more targeted parsing approaches would be safer, indicating different code quality standards apply to utility scripts versus production code.

Learnt from: lucasdellabella
PR: metriport/metriport#3941
File: packages/utils/src/hl7v2-notifications/unpack-patient-roster-identifier.ts:66-68
Timestamp: 2025-05-31T23:09:07.678Z
Learning: Scripts and utility tools may have relaxed code standards compared to production application code. Type safety improvements and other refactoring suggestions should be applied with less rigor to debugging scripts.

Learnt from: leite08
PR: metriport/metriport#3595
File: packages/utils/src/carequality/download-directory.ts:36-66
Timestamp: 2025-04-03T02:23:58.332Z
Learning: In the metriport codebase, utility scripts may not require the same level of error handling as production code, especially for simple or one-off tasks like in the CQ directory download script.

Learnt from: lucasdellabella
PR: metriport/metriport#3987
File: packages/utils/src/local-bulk-insert-patients.ts:216-219
Timestamp: 2025-06-10T13:41:50.425Z
Learning: For utility scripts and testing scripts in the packages/utils directory, prefer simpler error handling rather than detailed contextual error messages, as these are typically used internally and don't require the same level of robustness as production code.

Learnt from: keshavsaharia
PR: metriport/metriport#4099
File: packages/utils/src/surescripts/shared.ts:52-63
Timestamp: 2025-06-26T06:27:07.109Z
Learning: For utility scripts in packages/utils, comprehensive error handling (like try-catch blocks for S3 operations and JSON parsing) is not necessary, as immediate failures can be more helpful for developers running these one-off tools.

Learnt from: leite08
PR: metriport/metriport#3412
File: packages/utils/src/patient-import/send-webhook.ts:36-40
Timestamp: 2025-04-24T02:17:19.333Z
Learning: Utility scripts in the `packages/utils` directory should include proper error handling with try/catch blocks around their main functions, logging errors and exiting with non-zero status codes upon failure.

Learnt from: lucasdellabella
PR: metriport/metriport#3866
File: packages/core/src/command/hl7v2-subscriptions/hl7v2-roster-generator.ts:142-151
Timestamp: 2025-05-27T16:10:48.223Z
Learning: In packages/core/src/command/hl7v2-subscriptions/hl7v2-roster-generator.ts, the user prefers to let axios errors bubble up naturally rather than adding try-catch blocks that re-throw with MetriportError wrappers, especially when the calling code already has retry mechanisms in place via simpleExecuteWithRetries.

Learnt from: leite08
PR: metriport/metriport#4095
File: packages/commonwell-cert-runner/src/single-commands/document-parse.ts:14-24
Timestamp: 2025-07-16T18:48:28.251Z
Learning: For utility scripts in the packages/commonwell-cert-runner directory, comprehensive error handling (like try-catch blocks for file operations and JSON parsing) is not necessary, as these are certification testing tools rather than production code.

Learnt from: keshavsaharia
PR: metriport/metriport#4342
File: packages/core/src/external/quest/command/bundle/get-bundle.ts:0-0
Timestamp: 2025-08-11T16:17:40.913Z
Learning: In packages/core/src/external/quest/command/bundle/get-bundle.ts, the Quest lab conversion files in S3 are guaranteed to contain valid JSON after the existence check, so JSON.parse() error handling is not necessary as these files are generated by a controlled internal process.

Learnt from: thomasyopes
PR: metriport/metriport#4090
File: packages/core/src/command/conversion-fhir/conversion-fhir-cloud.ts:15-18
Timestamp: 2025-06-25T01:55:27.902Z
Learning: In the Metriport codebase, JSON.stringify operations on Lambda payload construction with ConverterRequest objects (containing string payload and FhirConverterParams) are safe and will not throw, as they contain only simple serializable data types.

Learnt from: leite08
PR: metriport/metriport#3991
File: packages/core/src/external/aws/s3.ts:621-630
Timestamp: 2025-06-10T22:20:21.203Z
Learning: User leite08 prefers that all review comments are written in English.

packages/lambdas/package.json (1)

65-65: Re: snowflake-sdk in shared lambdas package

Previous review flagged keeping snowflake-sdk here can bloat all lambda bundles. You mentioned it’s intentional in preparation for moving Step 3 to Lambda. Consider scoping it to the specific ingest lambda (or a dedicated layer) when that ships so other functions don’t carry it.

packages/utils/src/fhir/bundle/find_with_custom_filter.sh (2)

97-110: Use jq’s exit status directly instead of capturing output (SC2155).

This was previously noted; consider relying on jq -e’s exit code for robustness.

 check_file_with_filter() {
     local file="$1"
     local filter="$2"
-    
-    # Apply the custom jq filter to the file
-    local result=$(jq -e "$filter" "$file" 2>/dev/null)
-    
-    if [ "$result" = "true" ]; then
-        return 0  # File matches the filter
-    fi
-    
-    return 1  # File doesn't match the filter
+    # Apply the custom jq filter; truthy -> exit 0, false/null -> non-zero
+    if jq -e "$filter" "$file" >/dev/null 2>&1; then
+        return 0
+    fi
+    return 1
 }

---

112-118: Handle spaces/newlines in filenames with NUL-safe iteration.

This was previously suggested; using -print0 + read -d '' prevents word-splitting issues.

-# Find all JSON files in the directory and subdirectories up to maxdepth
-json_files=$(find "$DIRECTORY" -maxdepth "$MAXDEPTH" -name "*.json" -type f | sort)
-
-if [ -z "$json_files" ]; then
-    echo "No JSON files found in directory: $DIRECTORY"
-    exit 0
-fi
+scanned_count=0
@@
-# Process each JSON file
-for file in $json_files; do
+# Process each JSON file (NUL-delimited to be safe with spaces/newlines)
+while IFS= read -r -d '' file; do
+    scanned_count=$((scanned_count + 1))
@@
-done
+done < <(find "$DIRECTORY" -maxdepth "$MAXDEPTH" -type f -name "*.json" -print0)
+
+if [ "$scanned_count" -eq 0 ]; then
+    echo "No JSON files found in directory: $DIRECTORY"
+    exit 0
+fi

Also applies to: 129-147

packages/utils/src/analytics-platform/2-merge-csvs.ts (2)

169-171: Add validation for empty patient ID list

The function doesn't validate if any patient IDs were found, which could lead to silent failures downstream.

Apply this fix to validate the patient list:

   const patientIds = files.flatMap(file =>
     file.Prefix ? parsePatientIdFromFhirToCsvPatientPrefix(file.Prefix) : []
   );
+  
+  if (patientIds.length === 0) {
+    throw new Error(`No patient data found for fhirToCsvJobId: ${fhirToCsvJobId}`);
+  }
+  
   return patientIds;

---

121-121: Deduplication ID could exceed reasonable limits with many patient IDs

Using ptIdsOfThisRun.join(",") to generate the deduplication ID could create very long strings (up to 1,200 UUIDs = ~43KB), and if patient IDs contain commas, it could lead to collisions.

Consider using a hash-based approach for more reliable deduplication:

+import crypto from "crypto";

 // ... in the executeAsynchronously callback:
         await sqsClient.sendMessageToQueue(queueUrl, payloadString, {
           fifo: true,
-          messageDeduplicationId: createUuidFromText(ptIdsOfThisRun.join(",")),
+          // Create a deterministic hash of the sorted patient IDs for consistent deduplication
+          messageDeduplicationId: crypto
+            .createHash("sha256")
+            .update(JSON.stringify({ jobId: mergeCsvJobId, patientIds: [...ptIdsOfThisRun].sort() }))
+            .digest("hex")
+            .substring(0, 32), // Use first 32 chars for a valid dedup ID
           messageGroupId,

packages/core/src/external/aws/lambda.ts (3)

27-33: Use NodeHttpHandler for AWS SDK v3 client timeouts

The AWS SDK v3 doesn't support the httpOptions property. Network-level timeouts must be configured using a custom HTTP handler.

Import the handler and update the client construction:

+import { NodeHttpHandler } from "@smithy/node-http-handler";

 export function makeLambdaClientV3(region: string, timeoutInMillis?: number): LambdaClientV3 {
-  const lambdaClient = new LambdaClientV3({
-    region,
-    ...(timeoutInMillis ? { httpOptions: { timeout: timeoutInMillis } } : {}),
-  });
-  return lambdaClient;
+  const config: any = { region };
+  
+  if (timeoutInMillis) {
+    config.requestHandler = new NodeHttpHandler({
+      connectionTimeout: timeoutInMillis,
+      socketTimeout: timeoutInMillis,
+    });
+  }
+  
+  return new LambdaClientV3(config);
 }

---

97-106: Properly decode Uint8Array payload before JSON parsing

The V3 SDK returns Payload as a Uint8Array. Calling .toString() on it may not decode UTF-8 correctly.

Apply this fix to properly decode the payload:

 export function getLambdaErrorV3(result: InvokeCommandOutput): LambdaError | undefined {
   if (!result.Payload) return undefined;
   if (!isLambdaErrorV3(result)) return undefined;
-  const response = JSON.parse(result.Payload.toString());
+  const responseStr = Buffer.from(result.Payload as Uint8Array).toString("utf-8");
+  const response = JSON.parse(responseStr);
   return {
     errorType: response.errorType,
     errorMessage: response.errorMessage,
     log: logResultToString(result.LogResult),
   };
 }

---

249-249: Decode Uint8Array payload before returning

The return statement should decode the Uint8Array properly.

Apply this fix:

-  return result.Payload.toString();
+  return Buffer.from(result.Payload as Uint8Array).toString("utf-8");