packages/utils/src/surescripts/sftp-action.ts (3)

18-21: Add PHI/PII caution to the read example

read prints file contents to stdout (ascii). These files can contain PHI. Recommend adding a caution and a redirect example.

Apply this diff to enhance the docs:

@@
  * You can read a file from the remote SFTP server with:
  *
  * npm run surescripts -- sftp read /path/to/file
+ *
+ * Caution: `read` prints the full file contents to stdout. Files may contain PHI/PII.
+ * Prefer redirecting output to a secure location:
+ *   npm run surescripts -- sftp read /path/to/file > output.txt

---

14-17: Document list filters (--prefix, --contains) for discoverability

The CLI supports --prefix and --contains but the header block doesn’t mention them. Add examples to reduce guesswork.

Apply this diff to the docs block:

@@
  * You can list all files on the remote SFTP server with:
  *
  * npm run surescripts -- sftp list /path/to/directory
+ * 
+ * Options:
+ *   --prefix <prefix>   Filter keys by prefix
+ *   --contains <text>   Filter keys by substring
+ * Examples:
+ *   npm run surescripts -- sftp list /dir --prefix new/
+ *   npm run surescripts -- sftp list /dir --contains 2025-08-18

Also applies to: 72-79

---

28-31: Correct the @see target to the concrete file/path

If the intended reference is the S3-backed replica implementation, point to replica/s3.ts rather than replica.ts.

Apply this diff:

- * @see @metriport/core/external/sftp/replica.ts for more details.
+ * @see @metriport/core/external/sftp/replica/s3.ts for more details.

If the project has a replica index (e.g., replica/index.ts) that centralizes docs, use that instead.

packages/core/src/external/sftp/replica/s3.ts (1)

5-6: Consider adding brief JSDoc on why these fields are protected

Tiny clarity win for future maintainers.

Apply this diff to document intent:

 export class S3Replica implements SftpReplica {
-  protected readonly s3: S3Utils;
-  protected readonly bucketName: string;
+  /** S3 client utils. Protected to allow subclasses to reuse low-level operations (immutable). */
+  protected readonly s3: S3Utils;
+  /** Default bucket for this replica. Protected for subclass access (immutable). */
+  protected readonly bucketName: string;

packages/core/src/external/surescripts/schema/request.ts (1)

148-152: Dropped "N" looks correct; fix the inline comment casing.

Good alignment with the narrowed domain. Minor nit: the comment says "u - unknown" while the actual enum value is "U".

Apply this diff to correct the comment:

-  genderAtBirth: z.enum(["M", "F", "U"]), // u - unknown
+  genderAtBirth: z.enum(["M", "F", "U"]), // U - unknown

packages/core/src/external/surescripts/file/file-generator.ts (1)

25-31: Avoid duplicating SurescriptsGender; import the canonical type.

Defining a local union risks drift from the source type in ../types. Import and reuse the canonical SurescriptsGender.

Apply this diff within these lines:

-type SurescriptsGender = "M" | "F" | "U";
 const makeGenderDemographics = genderMapperFromDomain<SurescriptsGender>(
   {
     M: "M",
     F: "F",
-    O: "U",
+    O: "U",
     U: "U",
   },
   "U"
 );

And add this import at the top of the file (outside the selected range):

import type { SurescriptsGender } from "../types";

packages/core/src/external/surescripts/fhir/constants.ts (1)

26-26: Duplicate constant value with a different name may cause confusion.

HL7_CODE_SYSTEM_URL duplicates the value of PLAN_NETWORK_PCN_SYSTEM ("v2-0203"). Consider unifying under a single, clearly named constant (e.g., HL7_IDENTIFIER_TYPE_SYSTEM) and deprecating the domain-specific alias.

Happy to draft a small follow-up PR to replace usages and keep a transitional alias.

packages/core/src/external/surescripts/codes.ts (1)

70-75: Simplify alias resolution and avoid String#match; prefer normalization with RegExp#test

Micro clean-up for readability and fewer branches. No behavior change.

Apply this diff:

-export function getPaymentCodeName(code: PaymentCode): PaymentCodeName {
-  if (code.length === 1 && code.match(/^[1-7]$/)) {
-    return getPaymentCodeName(("0" + code) as PaymentCode);
-  }
-  return PAYMENT_CODE_NAME[code] ?? "Other";
-}
+export function getPaymentCodeName(code: PaymentCode): PaymentCodeName {
+  const normalized =
+    code.length === 1 && /^[1-7]$/.test(code) ? (("0" + code) as PaymentCode) : code;
+  return PAYMENT_CODE_NAME[normalized] ?? "Other";
+}

packages/core/src/external/surescripts/schema/response.ts (1)

226-229: Optional shim: Map legacy 'N' -> 'U' to prevent ingestion failures during transition

If there’s any chance of lingering 'N' values, add a small transformer to keep ingestion resilient while upstream completes the change.

Apply this diff:

-    fromSurescripts: fromSurescriptsEnum(["M", "F", "U"]),
+    fromSurescripts: fromSurescriptsGender,

And add this helper near the top of the file (after imports):

function fromSurescriptsGender(value: string): "M" | "F" | "U" {
  return value === "N" ? "U" : fromSurescriptsEnum(["M", "F", "U"])(value);
}

packages/core/src/external/surescripts/fhir/condition.ts (2)

39-51: Make Z-code check case-insensitive and add a brief “why” comment

Minor robustness and inline rationale (to match the guideline of documenting why).

Apply this diff:

-function getClinicalStatus(detail: ResponseDetail): Condition["clinicalStatus"] {
-  if (detail.diagnosisICD10Code?.startsWith("Z")) {
+function getClinicalStatus(detail: ResponseDetail): Condition["clinicalStatus"] {
+  /** Z-codes (e.g., encounters/contacts) are treated as active and confirmed for analytics. */
+  if (detail.diagnosisICD10Code?.toUpperCase().startsWith("Z")) {
     return {
       coding: [
         {
           system: CONDITION_CLINICAL_STATUS_URL,
           code: "active",
         },
       ],
     };
   }
   return undefined;
 }

---

53-65: Mirror the case-insensitive Z-code check and add rationale

Keep behavior consistent across both status helpers.

Apply this diff:

-function getVerificationStatus(detail: ResponseDetail): Condition["verificationStatus"] {
-  if (detail.diagnosisICD10Code?.startsWith("Z")) {
+function getVerificationStatus(detail: ResponseDetail): Condition["verificationStatus"] {
+  /** Z-codes are asserted as confirmed to improve downstream clinical signal. */
+  if (detail.diagnosisICD10Code?.toUpperCase().startsWith("Z")) {
     return {
       coding: [
         {
           system: CONDITION_VERIFICATION_STATUS_URL,
           code: "confirmed",
         },
       ],
     };
   }
   return undefined;
 }

packages/core/src/external/surescripts/fhir/medication-request.ts (2)

178-190: ReasonCode mapping is correct; consider including human-readable text if available

The ICD-10 mapping is correct. If the source detail includes a diagnosis description, consider adding CodeableConcept.text (and/or coding.display) to improve UX and downstream readability.

Would you like me to add a guarded text/display mapping if fields like detail.diagnosisICD10Description are present?

---

23-37: Add unit tests for new fields (identifier, reasonCode, pharmacy/performer)

Recommend tests covering:

• identifier presence/absence based on rxReferenceNumber
• reasonCode population with ICD-10 code
• pharmacy mapped to dispenseRequest.performer (after refactor)
• intent remains "order"

I can generate targeted Jest tests for these cases if helpful.

Also applies to: 49-51, 69-71, 75-94, 178-190

packages/utils/src/surescripts/send-facility-request.ts (2)

76-77: Check filename logging vs. actual path/extension handling.

The log appends “.csv” while the function receives csvOutput already. If writeSurescriptsIdentifiersFile also appends “.csv”, the message is correct; otherwise, it will be misleading.

Consider logging the exact path you pass to the writer:

-      log(`Wrote ${identifiers.length} identifiers to ${csvOutput}.csv`);
+      log(`Wrote ${identifiers.length} identifiers to ${csvOutput}`);

---

73-77: Optional: wrap the SFTP send and write in try/catch for clearer failure visibility.

As a CLI, letting errors bubble is fine, but a small try/catch improves operability and preserves the original error.

Apply this minimal diff:

-      log("Sending " + requests.length + " requests");
-      const identifiers = await client.sendBatchPatientRequest(requests);
-      log("Done writing facility requests");
-      writeSurescriptsIdentifiersFile(csvOutput, identifiers);
-      log(`Wrote ${identifiers.length} identifiers to ${csvOutput}.csv`);
+      log("Sending " + requests.length + " requests");
+      try {
+        const identifiers = await client.sendBatchPatientRequest(requests);
+        log("Done writing facility requests");
+        writeSurescriptsIdentifiersFile(csvOutput, identifiers);
+        log(`Wrote ${identifiers.length} identifiers to ${csvOutput}`);
+      } catch (error) {
+        log(`Error sending facility requests: ${(error as Error).message}`);
+        throw error;
+      }

packages/utils/src/surescripts/convert-customer-response.ts (1)

63-68: Validate and radix-parse the start/end indices to avoid NaN surprises.

parseInt without radix and no validation can yield unexpected slices (e.g., NaN → 0). Add radix and guard invalid values.

Apply this diff:

-      const startIndex = parseInt(start ?? "0");
-      const endIndex = end != null ? parseInt(end) : undefined;
+      const startIndex = Number.parseInt(start ?? "0", 10);
+      const endIndex = end != null ? Number.parseInt(end, 10) : undefined;
+      if (Number.isNaN(startIndex) || (end != null && Number.isNaN(endIndex!))) {
+        throw new Error("start and end must be integers");
+      }

packages/utils/src/surescripts/generate-csv.ts (1)

67-80: Avoid async scheduler overhead for a pure in-memory filter/map.

The worker only checks membership and pushes to an array; no awaits. Using executeAsynchronously adds overhead without benefit. Prefer a synchronous loop for determinism and simplicity.

Apply this diff:

-  await executeAsynchronously(
-    responses,
-    async response => {
-      if (patientIdSet.has(response.patientId)) {
-        identifiers.push({
-          transmissionId: response.transmissionId,
-          populationId: response.patientId,
-        });
-      }
-    },
-    {
-      numberOfParallelExecutions: 10,
-    }
-  );
+  for (const response of responses) {
+    if (patientIdSet.has(response.patientId)) {
+      identifiers.push({
+        transmissionId: response.transmissionId,
+        populationId: response.patientId,
+      });
+    }
+  }

packages/core/src/external/surescripts/replica.ts (2)

48-51: Convenience read-by-key helper is sensible.

Returning decompressed content matches getRawResponseFile behavior. Consider documenting that the key is expected to point to a gzipped object.

---

53-71: Parsing keys by fixed indices is brittle; prefer a format-checked parse.

The index math works for the current filename scheme, but a small drift (e.g., prefix change) will silently misparse. A regex against the relative key improves robustness and self-documents expectations.

Apply this refactor:

-  async listResponseFiles(): Promise<
-    Array<{ key: string; transmissionId: string; patientId: string }>
-  > {
-    const responseFiles = await this.listFileNames(INCOMING_NAME);
-    const startIndex = INCOMING_NAME.length + 1;
-    return responseFiles
-      .filter(key => {
-        return key.charAt(startIndex + 10) === "_" && key.charAt(startIndex + 47) === "_";
-      })
-      .map(key => {
-        const transmissionId = key.substring(startIndex, startIndex + 10);
-        const patientId = key.substring(startIndex + 11, startIndex + 47);
-        return {
-          key,
-          transmissionId,
-          patientId,
-        };
-      });
-  }
+  async listResponseFiles(): Promise<
+    Array<{ key: string; transmissionId: string; patientId: string }>
+  > {
+    const responseFiles = await this.listFileNames(INCOMING_NAME);
+    const startIndex = INCOMING_NAME.length + 1; // after "from_surescripts/"
+    return responseFiles
+      .map(key => {
+        const relative = key.substring(startIndex);
+        const match = /^([A-Za-z0-9]{10})_([0-9a-fA-F-]{36})_/.exec(relative);
+        if (!match) return undefined;
+        const [, transmissionId, patientId] = match;
+        return { key, transmissionId, patientId };
+      })
+      .filter(
+        (x): x is { key: string; transmissionId: string; patientId: string } => x !== undefined
+      );
+  }

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: keshavsaharia
PR: metriport/metriport#4045
File: packages/core/src/external/surescripts/fhir/coverage.ts:0-0
Timestamp: 2025-06-18T18:34:10.489Z
Learning: Coverage resources in Surescripts FHIR conversion are currently excluded from bundles to prevent skewing data lift metrics. The team plans to examine available insurance data thoroughly before including properly structured Coverage resources with mandatory FHIR R4 elements like beneficiary and payor references.

Learnt from: keshavsaharia
PR: metriport/metriport#4370
File: packages/core/src/external/quest/id-generator.ts:5-8
Timestamp: 2025-08-15T00:20:50.840Z
Learning: User keshavsaharia highly values rigorous mathematical analysis and comprehensive technical solutions that include both problem identification and complete remediation paths, particularly appreciating when statistical claims are verified and corrected with proper calculations.

Learnt from: keshavsaharia
PR: metriport/metriport#4020
File: packages/core/src/external/fhir/adt-encounters.ts:245-253
Timestamp: 2025-08-04T17:46:17.853Z
Learning: User keshavsaharia prefers lodash chains over native JavaScript methods for readability when working with complex data transformations, even when it involves using underscore imports and might have less robust error handling.

Learnt from: keshavsaharia
PR: metriport/metriport#4370
File: packages/shared/src/domain/patient.ts:5-5
Timestamp: 2025-08-15T00:00:45.080Z
Learning: The patientSchema in packages/shared/src/domain/patient.ts is used in a limited subsystem scope ("SS") and is not the same as other Patient schemas referenced throughout the codebase, making additive optional field changes like externalId safe and non-breaking.

Learnt from: keshavsaharia
PR: metriport/metriport#3885
File: packages/core/src/external/surescripts/schema/load.ts:65-71
Timestamp: 2025-05-30T03:42:53.380Z
Learning: In Surescripts schema files like `packages/core/src/external/surescripts/schema/load.ts`, the `key` property in field definitions is only used for direct one-to-one field mappings. When multiple fields are derived from the same source property but with different transformations (like extracting date vs. time portions), they should not have `key` properties as they represent transformations rather than direct mappings.

Learnt from: thomasyopes
PR: metriport/metriport#3970
File: packages/api/src/external/ehr/athenahealth/command/write-back/medication.ts:17-17
Timestamp: 2025-06-06T16:45:31.832Z
Learning: The writeMedicationToChart function in packages/api/src/external/ehr/athenahealth/command/write-back/medication.ts returns a response that is not currently used by any consumers, so changes to its return type are not breaking changes in practice.

Learnt from: lucasdellabella
PR: metriport/metriport#4020
File: packages/fhir-sdk/src/index.ts:308-308
Timestamp: 2025-08-04T17:59:05.674Z
Learning: In packages/fhir-sdk/src/index.ts, the FhirBundleSdk constructor intentionally mutates the input bundle's total property to ensure data consistency between bundle.total and bundle.entry.length. This is an accepted exception to the "avoid modifying objects received as parameter" guideline for cleanup purposes.

Learnt from: thomasyopes
PR: metriport/metriport#4090
File: packages/core/src/command/conversion-fhir/conversion-fhir-cloud.ts:30-30
Timestamp: 2025-06-25T01:55:42.627Z
Learning: In packages/core/src/command/conversion-fhir/conversion-fhir-cloud.ts, the FHIR converter Lambda endpoint is controlled by the team and guaranteed to return valid JSON in the expected Bundle<Resource> format, so JSON.parse() error handling and type validation are not necessary for this specific endpoint.

Learnt from: thomasyopes
PR: metriport/metriport#3608
File: packages/core/src/external/ehr/canvas/index.ts:451-469
Timestamp: 2025-04-21T17:07:30.574Z
Learning: The `getMetriportOnlyBundleByResourceType` method in CanvasApi returns `Promise<Bundle | undefined>` because the Metriport-only bundle is a computed artifact that may not exist yet. In contrast, `getBundleByResourceType` returns `Promise<Bundle>` because it can always fetch from the EHR API if the S3 cached bundle doesn't exist.

Learnt from: thomasyopes
PR: metriport/metriport#3882
File: packages/core/src/external/ehr/lambdas/get-bundle-by-resource-type/ehr-get-bundle-by-resource-type-cloud.ts:0-0
Timestamp: 2025-05-28T19:20:47.442Z
Learning: In packages/core/src/external/ehr/lambdas/get-bundle-by-resource-type/ehr-get-bundle-by-resource-type-cloud.ts, the EHR get bundle by resource type Lambda endpoint is guaranteed to return valid JSON, so JSON.parse() error handling is not necessary for this specific endpoint.

Learnt from: thomasyopes
PR: metriport/metriport#3788
File: packages/api/src/external/ehr/shared/utils/bundle.ts:83-93
Timestamp: 2025-05-08T19:41:36.533Z
Learning: In the Metriport codebase, the team prefers to let errors bubble up naturally in some cases rather than adding explicit error handling at every layer, as demonstrated in the refreshEhrBundle function in the bundle.ts file.

Learnt from: thomasyopes
PR: metriport/metriport#3882
File: packages/core/src/external/ehr/command/get-bundle-by-resource-type/ehr-get-bundle-by-resource-type-cloud.ts:27-49
Timestamp: 2025-05-28T19:22:09.281Z
Learning: In packages/core/src/external/ehr/command/get-bundle-by-resource-type/ehr-get-bundle-by-resource-type-cloud.ts, the EHR get bundle by resource type Lambda endpoint is guaranteed to return valid JSON, so JSON.parse() error handling is not necessary for this specific endpoint.

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: 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: RamilGaripov
PR: metriport/metriport#3686
File: docs/medical-api/fhir/extensions/chronicity.mdx:12-21
Timestamp: 2025-04-22T00:07:37.729Z
Learning: In Metriport's FHIR Condition chronicity extension, they use "http://hl7.org/fhir/StructureDefinition/condition-related" as the extension URL and "https://public.metriport.com/fhir/StructureDefinition/condition-chronicity.json" as the valueCoding.system.

Learnt from: keshavsaharia
PR: metriport/metriport#4033
File: packages/core/src/external/surescripts/fhir/patient.ts:29-36
Timestamp: 2025-06-17T01:51:12.930Z
Learning: In Surescripts response files, the patientFirstName and patientLastName fields in ResponseDetail are always set to valid strings and are required according to the specification, so defensive checks for undefined/empty values are not needed when constructing FHIR Patient names.

Learnt from: thomasyopes
PR: metriport/metriport#3885
File: packages/core/src/external/surescripts/client.ts:170-180
Timestamp: 2025-06-13T21:41:01.744Z
Learning: In packages/core/src/external/surescripts/client.ts, the leading slash in "/from_surescripts" used with listFileNamesWithPrefix for replica lookups is intentional, despite appearing inconsistent with other replica path references that don't use leading slashes.

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 SurescriptsSftpClient constructor intentionally uses config.publicKey as the SFTP password field and config.username as the SFTP username field. This is the expected configuration for Surescripts SFTP authentication.

Learnt from: keshavsaharia
PR: metriport/metriport#4075
File: packages/core/src/external/surescripts/fhir/medication-request.ts:76-83
Timestamp: 2025-06-25T18:42:07.231Z
Learning: In packages/core/src/external/surescripts/fhir/medication-request.ts, the getDispenseNote and getDosageInstruction functions are intentionally identical because the dashboard requires both the note and dosageInstruction fields of MedicationRequest to be populated with detail.directions for proper note display functionality.

Learnt from: keshavsaharia
PR: metriport/metriport#4045
File: packages/shared/src/interface/external/surescripts/plan-code.ts:1-3
Timestamp: 2025-06-18T17:22:24.597Z
Learning: In packages/shared/src/interface/external/surescripts/plan-code.ts, the getPlanCodeName function should return undefined for invalid plan codes rather than throwing errors, as per user preference for graceful error handling.