packages/commonwell-sdk/src/models/organization.ts (1)

104-107: Consider Aligning organizationSchemaWithoutNetworkInfo with Its Name or Renaming It

It looks like the current schema still exposes gateways and securityTokenKeyType, which may confuse anyone expecting all network-related fields to be omitted. Your ripgrep search didn’t surface any direct uses of the OrganizationWithoutNetworkInfo type in the codebase, so adjusting the omit or renaming the export should be low-risk.

Locations to review:

• packages/commonwell-sdk/src/models/organization.ts
  Lines 104–107 define:

  export const organizationSchemaWithoutNetworkInfo =
    organizationSchemaWithNetworkInfo.omit({
      networks: true,
      authorizationInformation: true,
    });
  export type OrganizationWithoutNetworkInfo =
    z.infer<typeof organizationSchemaWithoutNetworkInfo>;

Optional refactor suggestions:

• Option A — Omit all network fields to match the name:

 export const organizationSchemaWithoutNetworkInfo =
   organizationSchemaWithNetworkInfo.omit({
     networks: true,
-    authorizationInformation: true,
+    authorizationInformation: true,
+    gateways: true,
+    securityTokenKeyType: true,
   });

• Option B — Keep the current shape but rename for precision:

export const organizationSchemaWithoutNetworksAndAuth =
  organizationSchemaWithNetworkInfo.omit({
    networks: true,
    authorizationInformation: true,
  });
export type OrganizationWithoutNetworksAndAuth =
  z.infer<typeof organizationSchemaWithoutNetworksAndAuth>;

packages/shared/src/common/retry.ts (2)

42-47: Clarify the onError contract in docs (and consider passing attempt for richer context)

The doc states onError doesn’t affect shouldRetry/outcome and runs before shouldRetry. Today, if onError itself throws, it will derail the retry flow (see catch path). Either (a) harden the implementation to swallow/log onError errors (recommended — see another comment), or (b) update the docs to explicitly state that onError must not throw.

Doc tweak (if you harden the implementation):

   /**
-   * Function to be called when an error occurs. It doesn't change whether shouldRetry is called
-   * or not, nor does it change the result of that function.
-   * It's called before shouldRetry.
+   * Called once per failed attempt (catch path) before shouldRetry.
+   * It must not influence shouldRetry or the outcome.
+   * If it throws, the error is swallowed and logged; the retry flow proceeds.
+   * Not awaited.
    */

Optionally, consider including the attempt number in the callback for better observability:

-  onError?: (error: unknown) => void;
+  onError?: (error: unknown, attempt: number) => void;

If you do this, remember to pass attempt in the catch path and update tests. The existing tests that assert the first arg only will continue to pass.

---

7-10: Align defaultShouldRetry signature with declared type (minor consistency)

shouldRetry is invoked with (result, error, attempt). Default impl ignores attempt, but adding it to the signature improves consistency/IntelliSense and avoids accidental refactors that rely on parameter counts.

-function defaultShouldRetry<T>(_: T | undefined, error: unknown) {
+function defaultShouldRetry<T>(_: T | undefined, error: unknown, _attempt: number) {
   if (error) return true;
   return false;
}

packages/shared/src/common/__tests__/retry.test.ts (1)

118-188: New onError tests are solid; consider deterministic timing and add two edge cases

Great coverage validating per-attempt invocation, absence on success, and argument identity. Two enhancements:

1. Eliminate real sleeps for speed/stability by injecting getTimeToWait: () => 0 in these tests.
2. Add a test proving onError is not called when retries are driven by a retryable result (no thrown error).
3. If you adopt the onError guard in retry.ts, add a test ensuring an onError throw doesn’t break the retry flow.

Apply zero-wait to the new cases:

       await expect(async () =>
         executeWithRetries(fn, {
           initialDelay: 1,
           maxAttempts: 3,
+          getTimeToWait: () => 0,
         })
       ).rejects.toThrow();
...
       await expect(async () =>
         executeWithRetries(fn, {
           initialDelay: 1,
           maxAttempts: 3,
           onError,
+          getTimeToWait: () => 0,
         })
       ).rejects.toThrow();
...
       const resp = await executeWithRetries(fn, {
         initialDelay: 1,
         maxAttempts: 3,
         onError,
+        getTimeToWait: () => 0,
       });
...
       await expect(async () =>
         executeWithRetries(fn, {
           initialDelay: 1,
           maxAttempts: 2,
           onError,
+          getTimeToWait: () => 0,
         })
       ).rejects.toThrow();

Add “result-based retry doesn’t call onError”:

@@
     describe("onError", () => {
+      it("does not call onError when retrying due to a retryable result (no error thrown)", async () => {
+        const onError = jest.fn();
+        const retryableResult = faker.lorem.word();
+        const expectedResult = faker.lorem.word();
+        fn.mockImplementationOnce(() => retryableResult);
+        fn.mockImplementationOnce(() => expectedResult);
+        const resp = await executeWithRetries(fn, {
+          maxAttempts: 3,
+          shouldRetry: result => result === retryableResult,
+          getTimeToWait: () => 0,
+          onError,
+        });
+        expect(resp).toEqual(expectedResult);
+        expect(onError).not.toHaveBeenCalled();
+      });

Add “onError throw is contained” (only if you apply the guard in retry.ts):

@@
     describe("onError", () => {
+      it("swallows onError exceptions and continues retrying", async () => {
+        const onError = jest.fn(() => {
+          throw new Error("onError failure");
+        });
+        fn.mockImplementation(() => {
+          throw new Error("work error");
+        });
+        await expect(async () =>
+          executeWithRetries(fn, {
+            maxAttempts: 2,
+            getTimeToWait: () => 0,
+            onError,
+          })
+        ).rejects.toThrow("work error");
+        expect(onError).toHaveBeenCalledTimes(2);
+      });

Note: I intentionally kept arrow functions to match the existing test style, per our past learning that you prefer consistency within the file over isolated style changes.

packages/core/src/command/feature-flags/domain-ffs.ts (2)

306-309: Prefer includes(...) over some(...) for readability and consistency

Several helpers in this file use includes(cxId). Mirror that here to avoid unnecessary arrow functions and keep the style uniform.

 export async function isQuestFeatureFlagEnabledForCx(cxId: string): Promise<boolean> {
   const cxIdsWithQuestEnabled = await getCxsWithQuestFeatureFlag();
-  return cxIdsWithQuestEnabled.some(i => i === cxId);
+  return cxIdsWithQuestEnabled.includes(cxId);
 }

---

311-318: Use includes(...) and consider naming alignment with existing “With...Enabled” getters

• Swap some(i => i === cxId) for includes(cxId) for consistency/readability.
• Optional: for naming consistency with helpers like getCxsWithEpicEnabled, consider getCxsWithCommonwellV2Enabled as the getter name. Not blocking, but it keeps patterns predictable (noting prior team preference to keep file-local patterns consistent).

 export async function isCommonwellV2EnabledForCx(cxId: string): Promise<boolean> {
   const cxIdsWithCommonwellV2Enabled = await getCxsEnabledForCommonwellV2();
-  return cxIdsWithCommonwellV2Enabled.some(i => i === cxId);
+  return cxIdsWithCommonwellV2Enabled.includes(cxId);
 }

packages/commonwell-cert-runner/src/flows/document-consumption.ts (4)

11-11: OK on removing waitSeconds import; consider adding targeted polling to handle CW eventual consistency

Dropping the artificial wait can speed up the flow, but CommonWell responses (query/retrieve) are often eventually consistent. Without a brief, bounded poll, you'll see intermittent "no documents yet" or 404s immediately after patient creation. Recommend adding a small polling helper around the first query to reduce flakes.

Apply this diff to add a minimal, dependency-free poll and use it for the first query:

@@
-import { getMetriportPatientIdOrFail, logError } from "../util";
+import { getMetriportPatientIdOrFail, logError } from "../util";
@@
-    const documents = await queryDocuments(commonWell, encodedPatientId);
+    const documents = await poll(
+      () => queryDocuments(commonWell, encodedPatientId),
+      ds => ds.length > 0,
+      { tries: 6, delayMs: 1500 } // ~7.5s max
+    );
@@
 }
 
+/**
+ * Simple polling helper with linear backoff.
+ */
+async function poll<T>(
+  fn: () => Promise<T>,
+  isDone: (value: T) => boolean,
+  opts: { tries: number; delayMs: number }
+): Promise<T> {
+  const { tries, delayMs } = opts;
+  let last: T | undefined;
+  for (let i = 0; i < tries; i++) {
+    last = await fn();
+    if (isDone(last)) return last;
+    if (i < tries - 1) await new Promise(r => setTimeout(r, delayMs));
+  }
+  // return last attempt even if not "done" so callers can decide next step
+  return last as T;
+}

---

13-13: Use a filesystem-safe timestamp to avoid invalid folder names on Windows/macOS

toISOString() contains characters like ":" that break on Windows. Format the timestamp to a safe pattern.

Apply:

-const runTimestamp = buildDayjs().toISOString();
+const runTimestamp = buildDayjs().format("YYYYMMDD-HHmmss");

---

124-129: File path safety, cross-platform paths, and variable shadowing

• Potentially unsafe docId (could contain ":"/"/" etc.) is used as filename.
• Paths are manually concatenated; prefer path.join for cross-platform support.
• contentType is shadowed (declared twice), which is easy to misread.

Refactor to sanitize the ID, use path.join, and disambiguate types.

Apply:

+import path from "path";
@@
-  const outputDir = `${outputBaseDir}/${runTimestamp}`;
+  const outputDir = path.join(outputBaseDir, runTimestamp);
   fs.mkdirSync(outputDir, { recursive: true });
-  const fileNamePrefix = `${outputDir}/${docId}`;
+  const safeDocId = docId.replace(/[^a-zA-Z0-9._-]/g, "_");
+  const fileNamePrefix = path.join(outputDir, safeDocId);
   const docRefFileName = `${fileNamePrefix}.docRef.json`;
   const contentsFileName = `${fileNamePrefix}.contents${fileExtension}`;
@@
-    if (contentsFileName.endsWith(".bin")) {
-      const newFileExtension = getFileExtension(contentType);
+    if (contentsFileName.endsWith(".bin")) {
+      const newFileExtension = getFileExtension(contentType);
       fs.renameSync(contentsFileName, `${contentsFileName.replace(".bin", "")}${newFileExtension}`);
     }

And disambiguate content types:

-  const contentType = content?.attachment.contentType ?? "application/octet-stream";
-  const fileExtension = getFileExtension(contentType);
+  const declaredContentType = content?.attachment.contentType ?? "application/octet-stream";
+  const fileExtension = getFileExtension(declaredContentType);
@@
-    const { contentType, size } = await commonWell.retrieveDocument(url, outputStream);
+    const { contentType: retrievedContentType, size } = await commonWell.retrieveDocument(
+      url,
+      outputStream
+    );
@@
-      const newFileExtension = getFileExtension(contentType);
+      const newFileExtension = getFileExtension(retrievedContentType);

Also applies to: 139-142, 150-154

---

133-146: Ensure the write stream is closed on failures to avoid FD leaks

If retrieveDocument throws before closing the stream, we may leak a file descriptor. Close the stream in a finally block.

Apply:

-  try {
+  try {
     const { contentType, size } = await commonWell.retrieveDocument(url, outputStream);
     console.log(
       `>>> Downloaded document ${docId}, size: ${size} bytes, contentType: ${contentType}`
     );
     if (contentsFileName.endsWith(".bin")) {
       const newFileExtension = getFileExtension(contentType);
       fs.renameSync(contentsFileName, `${contentsFileName.replace(".bin", "")}${newFileExtension}`);
     }
-  } catch (error) {
+  } catch (error) {
     console.log(`Error retrieving document ${docId}: ${errorToString(error)}`);
     return false;
+  } finally {
+    try {
+      outputStream.close();
+    } catch {
+      // ignore
+    }
   }

packages/commonwell-cert-runner/src/flows/link-management.ts (4)

8-8: OK removing artificial waits; add bounded polling around eventually-consistent steps

Linking/unlinking/reset operations often require a short propagation before reads reflect the new state. Replace fixed sleeps with small, bounded polls around:

• 2.1.2 get links after create
• 2.2.2 get links / 2.2.3 probable links
• 2.3.3 get links after link
• 2.4.2 get links after unlink
• 2.5.2 get links after reset

This preserves speed but avoids flakiness.

Example (apply similarly to other reads):

-const resp_2_3_3 = await commonWell.getPatientLinksByPatientId(
-  patientWithProbableLinksIdEnconded
-);
+const resp_2_3_3 = await poll(
+  () => commonWell.getPatientLinksByPatientId(patientWithProbableLinksIdEncoded),
+  r => (r.Patients?.length ?? 0) > 0,
+  { tries: 5, delayMs: 1000 }
+);

Helper to add at bottom of the file (or import from a util if you already have one):

async function poll<T>(
  fn: () => Promise<T>,
  isDone: (value: T) => boolean,
  opts: { tries: number; delayMs: number }
): Promise<T> {
  const { tries, delayMs } = opts;
  let last: T | undefined;
  for (let i = 0; i < tries; i++) {
    last = await fn();
    if (isDone(last)) return last;
    if (i < tries - 1) await new Promise(r => setTimeout(r, delayMs));
  }
  return last as T;
}

---

60-67: Fix typo: Enconded → Encoded

Minor naming issue that hurts readability and can lead to future mistakes.

Apply:

-const patientWithProbableLinksIdEnconded = encodeToCwPatientId({
+const patientWithProbableLinksIdEncoded = encodeToCwPatientId({
   patientId: patientWithProbableLinksId,
   assignAuthority: commonWell.oid,
 });
@@
-const patientWithProbableLinks = await commonWell.getPatient(
-  patientWithProbableLinksIdEnconded
-);
+const patientWithProbableLinks = await commonWell.getPatient(
+  patientWithProbableLinksIdEncoded
+);
@@
-const resp_2_2_2 = await commonWell.getPatientLinksByPatientId(
-  patientWithProbableLinksIdEnconded
-);
+const resp_2_2_2 = await commonWell.getPatientLinksByPatientId(
+  patientWithProbableLinksIdEncoded
+);
@@
-const resp_2_2_2x = await commonWell.getPatientLinksByPatientId(
-  patientWithProbableLinksIdEnconded
-);
+const resp_2_2_2x = await commonWell.getPatientLinksByPatientId(
+  patientWithProbableLinksIdEncoded
+);
@@
-const resp_2_3_3 = await commonWell.getPatientLinksByPatientId(
-  patientWithProbableLinksIdEnconded
-);
+const resp_2_3_3 = await commonWell.getPatientLinksByPatientId(
+  patientWithProbableLinksIdEncoded
+);
@@
-const resp_2_4_2 = await commonWell.getPatientLinksByPatientId(
-  patientWithProbableLinksIdEnconded
-);
+const resp_2_4_2 = await commonWell.getPatientLinksByPatientId(
+  patientWithProbableLinksIdEncoded
+);
@@
-const resp_2_5_2 = await commonWell.getPatientLinksByPatientId(
-  patientWithProbableLinksIdEnconded
-);
+const resp_2_5_2 = await commonWell.getPatientLinksByPatientId(
+  patientWithProbableLinksIdEncoded
+);

Also applies to: 72-80, 83-90, 133-137, 148-153, 167-172

---

44-45: PII-heavy logs — consider masking in shared environments

The responses include PHI/PII (names, addresses, DOB). If this runner might be used beyond local dev, consider masking or reducing payload logs, or gating them behind an env flag.

Also applies to: 68-71, 124-131, 167-172

---

178-193: Optional: delete patients concurrently to reduce teardown time

Non-blocking improvement; cleanup can be parallelized with Promise.allSettled.

Apply:

-    const uniquePatientIds = uniq(patientIds);
-    for (const metriportPatientId of uniquePatientIds) {
-      try {
-        const patientId = encodeToCwPatientId({
-          patientId: metriportPatientId,
-          assignAuthority: commonWell.oid,
-        });
-        await commonWell.deletePatient(patientId);
-        console.log(`>>> Patient deleted: ${metriportPatientId}`);
-      } catch (err) {
-        console.log(`>>> Patient NOT deleted: ${metriportPatientId}`);
-        // intentionally ignore,
-      }
-    }
+    const uniquePatientIds = uniq(patientIds);
+    await Promise.allSettled(
+      uniquePatientIds.map(async metriportPatientId => {
+        try {
+          const patientId = encodeToCwPatientId({
+            patientId: metriportPatientId,
+            assignAuthority: commonWell.oid,
+          });
+          await commonWell.deletePatient(patientId);
+          console.log(`>>> Patient deleted: ${metriportPatientId}`);
+        } catch {
+          console.log(`>>> Patient NOT deleted: ${metriportPatientId}`);
+        }
+      })
+    );

packages/commonwell-cert-runner/README.md (2)

62-66: Clarify naming: “Member ID and Root OID”

Since the runner now expects ROOT_OID, consider updating the bullet to avoid confusion with CW_MEMBER_OID.

Apply:

-**Member Organization (Service Adopter):**
+**Member Organization (Service Adopter):**
@@
-- Member ID and OID
+- Member ID and Root OID

---

125-127: Ensure README’s env var matches the code

The README in packages/commonwell-cert-runner/README.md still shows CW_ORG_ID, but the runner’s code in src/env.ts reads CW_ORG_OID. Aligning the docs with the implementation will prevent confusion for users.

• File: packages/commonwell-cert-runner/README.md
Lines: 125–127

Apply:

-# Optional: Use existing organization instead of creating new one
-CW_ORG_ID=1.2.3.4.5.678.5.123
+# Optional: Use existing organization instead of creating new one
+CW_ORG_OID=1.2.3.4.5.678.5.123

packages/commonwell-cert-runner/src/index.ts (2)

51-51: Track failure label consistently; consider a tiny naming tweak

You push "initiatorOnlyOrgManagement" into failedFlows while the function is named orgManagementInitiatorOnly. Rename for consistency across code/searchability.

Apply within this line:

- await orgManagementInitiatorOnly().catch(() => failedFlows.push("initiatorOnlyOrgManagement"));
+ await orgManagementInitiatorOnly().catch(() => failedFlows.push("orgManagementInitiatorOnly"));

Optional: to reduce repetition of .catch(() => failedFlows.push(...)) in subsequent lines, consider a small helper:

// outside the changed lines, add:
function recordFailure<T>(name: string) {
  return (err: unknown) => {
    failedFlows.push(name);
  };
}

// then use:
// await patientManagement(commonWell).catch(recordFailure("patientManagement"));

---

57-63: Use console.log for success path (nit)

Using console.error for the “All flows passed!” message is slightly misleading and may trip log parsers. Suggest switching to console.log for the success branch.

Outside selected lines, suggested shape:

if (failedFlows.length < 1) {
  console.log(`\n>>> >>> All flows passed! <<< <<<\n`);
  process.exit(0);
} else {
  console.error(`\n>>> >>> Failed flows:\n- ${failedFlows.join("\n- ")}\n`);
  process.exit(1);
}

packages/commonwell-cert-runner/src/flows/contribution/document-reference.ts (2)

23-23: Prefer startsWith and a constant for the OID prefix to avoid false positives and magic strings

Checking for "urn:oid:" with includes() can match unintended positions; startsWith() is safer. Also, avoid magic strings per guidelines—use a constant.

Apply within this line:

-        "source": "${orgId.includes("urn:oid:") ? orgId : `urn:oid:${orgId}`}"
+        "source": "${orgId.startsWith(OID_PREFIX) ? orgId : `${OID_PREFIX}${orgId}`}"

And add near the imports (outside the changed lines):

const OID_PREFIX = "urn:oid:";

---

17-99: Consider building the DocumentReference as an object instead of JSON string (optional)

The JSON-as-string approach makes escaping and type-safety harder. Building an object and returning it avoids parse costs and reduces risk of accidental quoting bugs.

packages/commonwell-cert-runner/src/payloads.ts (3)

24-25: Root OID import is fine; add guardrail if env is missing

If rootOID is empty/misconfigured, generated IDs will be malformed (e.g., "undefined.5.XXXX"). Consider validating rootOID early and failing fast in this package.

Outside the changed lines, consider:

if (!rootOID) {
  throw new Error("ROOT_OID is required to build CommonWell IDs");
}

---

39-41: makeOrgId now composes from rootOID — add normalization and minimal validation (optional)

If callers accidentally pass an orgId that already includes the root (or a prefix), we may double-embed segments. Consider validating the orgId parameter length/format, or at least document that orgId must be a suffix token.

Example minimal guard within these lines:

 export function makeOrgId(orgId?: string): string {
   const org = orgId ?? makeId();
-  return `${rootOID}.${ORGANIZATION}.${org}`;
+  // Ensure no accidental prefixing (e.g., passing a full OID)
+  const suffix = org.includes(".") ? org.split(".").pop()! : org;
+  return `${rootOID}.${ORGANIZATION}.${suffix}`;
 }

---

188-192: Use buildDayjs() per codebase guidelines (optional)

Guidelines recommend using buildDayjs() rather than direct dayjs(...) to control parsing/timezone behavior. If available in this package, consider switching; otherwise, ignore.

Illustrative change (outside the changed lines, adjust import path as appropriate):

import { buildDayjs } from "@metriport/shared/common/time";
// ...
const validFrom = buildDayjs(x509.validFrom).toString();
const validTo = buildDayjs(x509.validTo).toString();

packages/commonwell-cert-runner/src/flows/contribution/contribution-server.ts (2)

56-56: Minor: prefer out().log or a consistent logger (optional)

For non-util packages we generally avoid console.log. Given this is a test tool, it may be acceptable—flagging for awareness only.

---

31-32: Duplicate express.json middleware (nit)

express.json is registered twice (Lines 31-32 and 139). Keep a single registration at the top.

Outside the selected lines, remove the later app.use(express.json({ limit: "2mb" }));.

Also applies to: 139-139

packages/commonwell-cert-runner/src/flows/org-management-initiator-only.ts (6)

31-33: Avoid sending empty string for securityTokenKeyType; delete the field instead

Empty-stringing enum-like fields is brittle and can be rejected by schema validation. Prefer deleting the property so it’s treated as “unset”. Same for updates.

Please confirm the CommonWell API accepts absent securityTokenKeyType but rejects empty strings. If absent is not allowed, consider using undefined rather than "".

-    initiatorOnlyOrgCreate.securityTokenKeyType = "";
-    delete initiatorOnlyOrgCreate.authorizationInformation;
+    delete initiatorOnlyOrgCreate.securityTokenKeyType;
+    delete initiatorOnlyOrgCreate.authorizationInformation;
@@
-    initiatorOnlyOrg.securityTokenKeyType = "";
-    if ("authorizationInformation" in initiatorOnlyOrg) {
-      delete initiatorOnlyOrg.authorizationInformation;
-    }
+    delete initiatorOnlyOrg.securityTokenKeyType;
+    delete initiatorOnlyOrg.authorizationInformation;

Also applies to: 51-54

---

29-33: Ensure initiator-only semantics in networks.purposeOfUse

makeOrganization() enables responder by default. For an initiator-only org, flip flags accordingly so behavior matches the label and avoids accidental responder configuration.

Please verify against the CommonWell spec whether initiator-only requires queryResponder=false and queryInitiatorOnly=true.

     const initiatorOnlyOrgCreate = makeOrganization();
     initiatorOnlyOrgCreate.gateways = [];
-    initiatorOnlyOrgCreate.securityTokenKeyType = "";
-    delete initiatorOnlyOrgCreate.authorizationInformation;
+    delete initiatorOnlyOrgCreate.authorizationInformation;
+    // enforce initiator-only behavior
+    initiatorOnlyOrgCreate.networks?.forEach(n =>
+      n.purposeOfUse?.forEach(p => {
+        p.queryResponder = false;
+        p.queryInitiatorOnly = true;
+      })
+    );

---

35-38: Gate multi-line JSON logs behind a VERBOSE flag

Pretty-printed JSON produces multi-line logs, which violate house style and can flood CI. Keep terse logs by default and enable detailed logs only when needed.

-    console.log(">>> Response: " + JSON.stringify(respCreateInitiatorOnly, null, 2));
+    if (process.env.VERBOSE === "1") {
+      console.log(">>> Response: " + JSON.stringify(respCreateInitiatorOnly));
+    } else {
+      console.log(">>> Response: created orgId=" + respCreateInitiatorOnly.organizationId);
+    }
@@
-    console.log(">>> Response: " + JSON.stringify(respGetInitiatorOnly, null, 2));
+    process.env.VERBOSE === "1" &&
+      console.log(">>> Response: " + JSON.stringify(respGetInitiatorOnly));
@@
-    console.log(">>> Response: " + JSON.stringify(respUpdateInitiatorOnly, null, 2));
+    process.env.VERBOSE === "1" &&
+      console.log(">>> Response: " + JSON.stringify(respUpdateInitiatorOnly));

Also applies to: 41-44, 56-58

---

59-66: Preserve root cause and avoid “[object Object]” when aggregating errors

Push stringified errors, and include the original errors as cause so the stack is retained upstream.

-  } catch (error) {
-    console.log(`Error (txId ${commonWellMember.lastTransactionId}): ${errorToString(error)}`);
-    errors.push(error);
-  }
+  } catch (error) {
+    const errStr = errorToString(error);
+    console.log(`Error (txId ${commonWellMember.lastTransactionId}): ${errStr}`);
+    errors.push(errStr);
+  }
@@
-  if (errors.length > 0) {
-    throw new Error(`Failed to run org management initiator only flow: ${errors.join(", ")}`);
-  }
+  if (errors.length > 0) {
+    const msg = `Failed to run org management initiator only flow: ${errors.join(", ")}`;
+    throw new Error(msg, { cause: new AggregateError(errors) });
+  }

---

48-49: Guard against missing locations

makeOrganization() currently sets one location, but a defensive guard prevents runtime errors if payloads change.

-    initiatorOnlyOrg.locations[0].city = faker.location.city();
+    if (!initiatorOnlyOrg.locations?.[0]) {
+      throw new Error("Organization payload missing at least one location");
+    }
+    initiatorOnlyOrg.locations[0].city = faker.location.city();

---

7-9: Unused export in this module

OrgManagementResponse is defined but not used here. Consider removing or moving it next to the consumer that returns it.

packages/commonwell-cert-runner/src/flows/org-management.ts (2)

42-42: Trim env-provided OID to avoid accidental whitespace issues

Small hardening so an env like " 1.2.3 " doesn't bypass the reuse branch.

-    let orgId: string | undefined = existingOrgOid;
+    let orgId: string | undefined = existingOrgOid?.trim();

---

48-50: Prefer concise logs and reuse the same banner format as the initiator-only flow

Keep banners consistent and avoid multi-line payload logs in this section. Consider reusing a helper to print TxId + key identifiers.

packages/commonwell-cert-runner/src/env.ts (1)

3-3: Optional: Rename rootOID to rootOid in commonwell-cert-runner for camel-case consistency

To align with the existing “Oid” pattern (e.g. existingOrgOid), update the constant and all its local references in the commonwell-cert-runner package:

• packages/commonwell-cert-runner/src/env.ts

-export const rootOID = getEnvOrFail("ROOT_OID");
+export const rootOid = getEnvOrFail("ROOT_OID");

• packages/commonwell-cert-runner/src/payloads.ts
– Update the import and usages of rootOID

-import { rootOID } from "./env";
+import { rootOid } from "./env";

   // …
-  payload.root = rootOID;
+  payload.root = rootOid;

- return `${rootOID}.${ORGANIZATION}.${org}`;
+ return `${rootOid}.${ORGANIZATION}.${org}`;

No changes are required to the uppercase environment-variable name (ROOT_OID) or to references outside of the cert-runner (e.g., SYSTEM_ROOT_OID in other packages).

packages/commonwell-cert-runner/src/flows/document-contribution.ts (2)

123-127: Gate verbose, pretty JSON logs

These logs are multi-line and can be very large. Mirror the VERBOSE gating used elsewhere.

-    console.log(">>> 2.2 Response: " + JSON.stringify(resp_2_2, null, 2));
+    process.env.VERBOSE === "1" &&
+      console.log(">>> 2.2 Response: " + JSON.stringify(resp_2_2));
@@
-    console.log(">>> 2.3 Response: " + JSON.stringify(resp_2_3, null, 2));
+    process.env.VERBOSE === "1" &&
+      console.log(">>> 2.3 Response: " + JSON.stringify(resp_2_3));

Also applies to: 139-143

---

149-149: Guard linking against missing link URLs

getProbableLinksById can return entries without Links.Link. Skip undefined URLs to avoid 4xx from linkPatients.

-    await linkPatients(commonWellContributor, firstPatientIdEncoded);
+    await linkPatients(commonWellContributor, firstPatientIdEncoded); // internally skip empty URLs

If linkPatients doesn’t already guard, add:

if (!urlToLink) {
  console.log(">>> Skipping link: empty URL");
  continue;
}

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: lucasdellabella
PR: metriport/metriport#3866
File: packages/api/src/routes/internal/medical/organization.ts:95-98
Timestamp: 2025-05-27T15:54:47.774Z
Learning: The conversion of the GET `/internal/organization` endpoint from returning a single organization to returning an array of organizations (batch retrieval) in `packages/api/src/routes/internal/medical/organization.ts` was an intentional breaking change planned by the team.

Learnt from: RamilGaripov
PR: metriport/metriport#4176
File: packages/fhir-converter/src/lib/handlebars-converter/handlebars-helpers.js:296-320
Timestamp: 2025-07-17T21:24:37.077Z
Learning: RamilGaripov prefers to maintain consistency with existing patterns in a file rather than making isolated fixes to coding guideline violations like var vs const declarations, when the pattern is already established throughout the codebase.

Learnt from: leite08
PR: metriport/metriport#3953
File: packages/core/src/command/consolidated/search/fhir-resource/__tests__/search-consolidated-setup.ts:28-30
Timestamp: 2025-06-03T21:02:23.374Z
Learning: The `makePatient()` function returns `PatientWithId` type which requires the `id` field to be present, ensuring `patient.id` is never undefined.

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#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: lucasdellabella
PR: metriport/metriport#3866
File: packages/api/src/routes/internal/medical/organization.ts:95-98
Timestamp: 2025-05-27T15:54:47.774Z
Learning: The conversion of the GET `/internal/organization` endpoint from returning a single organization to returning an array of organizations (batch retrieval) in `packages/api/src/routes/internal/medical/organization.ts` was an intentional breaking change planned by the team.

Learnt from: leite08
PR: metriport/metriport#4095
File: packages/commonwell-cert-runner/src/flows/org-management.ts:79-85
Timestamp: 2025-07-16T19:02:26.395Z
Learning: In packages/commonwell-cert-runner/src/flows/org-management.ts, the silent error handling in the certificate retrieval catch block is intentional to allow the certification flow to continue processing even when certificate operations fail, as this is a testing tool rather than production code.

Learnt from: RamilGaripov
PR: metriport/metriport#4434
File: packages/core/src/command/feature-flags/domain-ffs.ts:311-318
Timestamp: 2025-08-21T16:24:56.958Z
Learning: In the Metriport codebase, feature flag helper functions in packages/core/src/command/feature-flags/domain-ffs.ts follow a consistent pattern with getCxsWithXXXFeatureFlag() and isXXXEnabledForCx() functions, and individual unit tests are not added for each pair since they follow the same reliable pattern used throughout the codebase.

Learnt from: leite08
PR: metriport/metriport#3550
File: packages/api/src/external/commonwell/__tests__/patient.test.ts:5-5
Timestamp: 2025-04-01T20:57:29.282Z
Learning: PR #3574 is the follow-up PR that moves the domain feature flags from packages/api/src/aws/app-config.ts to packages/core/src/command/feature-flags/domain-ffs.ts.

Learnt from: leite08
PR: metriport/metriport#3550
File: packages/api/src/external/commonwell/__tests__/patient.test.ts:5-5
Timestamp: 2025-04-01T20:57:29.282Z
Learning: PR #3574 is the follow-up PR that moves the domain feature flags from app-config.ts to the domain folder in packages/core.

Learnt from: leite08
PR: metriport/metriport#3550
File: packages/api/src/external/commonwell/__tests__/patient.test.ts:5-5
Timestamp: 2025-04-01T19:59:22.396Z
Learning: The file at `packages/api/src/aws/app-config.ts` contains domain feature flags (not AWS AppConfig specific functionality) and is planned to be moved to the domain folder in packages/core in a future PR.

packages/shared/src/common/retry.ts (1)

90-93: LGTM: onError is correctly threaded through options

Wiring the new onError option alongside the other knobs keeps the API coherent. No concerns here.

packages/core/src/command/feature-flags/ffs-on-dynamodb.ts (1)

68-69: LGTM: default initialized for new flag

cxsEnabledForCommonwellV2 is correctly added to initialFeatureFlags with { enabled: false, values: [] }. This aligns with the new schema and prevents undefined access when no record exists.

packages/api/src/shared/config.ts (1)

240-243: Deprecation annotation approved; found 6 call sites—scope removal under ENG-554

Deprecating Config.getCWMemberOID() is correct. Before we remove it, we need to migrate each use. I’ve located six references:

• packages/api/src/external/commonwell-v2/api.ts
– Line 31: const isMemberAPI = orgOID === Config.getCWMemberOID();
– Line 60: const isMemberAPI = [Config.getCWMemberOID(), Config.getSystemRootOID()].includes(orgOID);

• packages/api/src/external/commonwell-v1/organization.ts
– Line 98: makeCommonWellAPI(Config.getCWMemberOrgName(), Config.getCWMemberOID());
– Line 124: makeCommonWellAPI(Config.getCWMemberOrgName(), Config.getCWMemberOID());
– Line 160: makeCommonWellAPI(Config.getCWMemberOrgName(), Config.getCWMemberOID());

• packages/api/src/external/commonwell-v1/api.ts
– Line 56: const isMemberAPI = orgOID === Config.getCWMemberOID();

Next steps:

1. Decide how each call should be updated (e.g., inline the env var or introduce a new accessor).
2. Schedule and track these migrations under ENG-554.
3. Once all usages are replaced, remove getCWMemberOID().

No code changes required in this PR—just tracking and planning.

packages/commonwell-cert-runner/README.md (1)

99-108: ROOT_OID doc update aligns with code migration

This matches the runner’s shift from memberOID to rootOID. Looks good.

packages/commonwell-cert-runner/src/index.ts (1)

9-9: Wiring the initiator-only flow is correct and side-effect free

Import looks good and keeps the new flow decoupled from the others. No exported API changes here.

packages/commonwell-cert-runner/src/flows/contribution/document-reference.ts (1)

28-29: Ensure orgId is a bare OID and strip any urn:oid: prefix for FHIR IDs

The makeDocumentReference helper currently uses orgId verbatim for both the contained Organization’s id and the author reference:

• "id": "${orgId}"
• "reference": "#${orgId}"

Because FHIR Resource.id must match [A-Za-z0-9\-\.]{1,64} and must not include the urn:oid: prefix, please:

• Verify at each call site of makeDocumentReference that orgId is always the bare dotted OID (e.g. 2.16.840.1.113883…) and never already wrapped in urn:oid:.
• To guard against accidental prefixes, normalize at the top of the function:

  import { stripUrnPrefix } from "@metriport/core/util/urn";
  const pureOrgId = stripUrnPrefix(orgId);

• Then update the JSON template to use pureOrgId:

    "contained": [
      {
        "resourceType": "Organization",

•   "id": "${orgId}",
  

•   "id": "${pureOrgId}",
    "name": "${orgName}"
  }
  

  ],
  "author": [
  {

•   "reference": "#${orgId}",
  

•   "reference": "#${pureOrgId}",
    "type": "Organization"
  }
  

  ],

This ensures that even if a URN-wrapped OID is passed in, the contained resource IDs and references remain valid.

packages/commonwell-cert-runner/src/single-commands/init-contrib-server.ts (2)

6-6: Env rename alignment looks correct

Swapping to existingOrgOid matches the broader migration away from memberOID; import set is correct.

---

13-19: Validate and/or strip the urn:oid: prefix from CW_ORG_OID

We confirmed that:

• In packages/commonwell-cert-runner/src/env.ts (line 22),

  export const existingOrgOid = getEnv("CW_ORG_OID");

  is pulled directly from the environment with no validation or mutation.

• In packages/commonwell-cert-runner/src/single-commands/init-contrib-server.ts (lines 24–27), both

  oid: org.organizationId,
  homeCommunityId: org.organizationId,

  are set from that same value.

Because there’s currently no logic to strip a leading urn:oid: prefix, if CW_ORG_OID includes it, both oid and homeCommunityId will carry the prefix—potentially invalidating downstream identifiers.

• Option A: Ensure the CW_ORG_OID environment variable is always provided as a bare dotted OID (e.g., 1.2.840.113619.6.3.3.1), without any urn:oid: prefix.
• Option B: Add normalization code immediately after reading the env var, for example:

const rawOid = getEnv("CW_ORG_OID");
const organizationId = rawOid.replace(/^urn:oid:/, "");
if (!organizationId) throw new Error("Organization ID is required");
// …

packages/commonwell-cert-runner/src/flows/contribution/contribution-server.ts (2)

11-11: Env import updates align with the new config shape

Using contribServerPort and contribServerUrl is consistent with the rest of the migration.

---

158-161: contribServerPort is already numeric – no change needed

We’ve confirmed that contribServerPort is initialized as

export const contribServerPort = parseInt(getEnvOrFail("CONTRIB_SERVER_PORT"));

so its type is number (not a string) when passed into app.listen . In our codebase, we rely on getEnvOrFail to guarantee required variables and intentionally omit explicit NaN‐checks after parseInt per established patterns . No additional coercion or guard is necessary here.

packages/commonwell-cert-runner/src/env.ts (1)

22-24: New optional OID envs look good

The additions for existingOrgOid, existingInitiatorOnlyOrgOid, and contribExistingOrgOid fit the flows that now support org reuse.

Also applies to: 25-25

packages/commonwell-cert-runner/src/flows/document-contribution.ts (1)

85-101: Env-driven consumer Org reuse is a solid improvement

Good fallback to create-and-wire certs when the OID isn’t provided.