[MAIN] [STRATCONN] Added validation on external_id of character 255

[AnkitSegment]

A summary of your pull request, including the what change you're making and why.

In this PR, a validation was added on Klaviyo's external_id where the external_id should not be greater than 255 characters.

Ref docs (Klaviyo): https://developers.klaviyo.com/en/reference/bulk_import_profiles

Testing

Include any additional information about the testing you have completed to
ensure your changes behave as expected. For a speedy review, please check
any of the tasks you completed below during your testing.

• Added unit tests for new functionality
• Tested end-to-end using the local server
• [If destination is already live] Tested for backward compatibility of destination. Note: New required fields are a breaking change.
• [Segmenters] Tested in the staging environment
• [Segmenters] [If applicable for this change] Tested for regression with Hadron.

Security Review

Please ensure sensitive data is properly protected in your integration.

• Reviewed all field definitions for sensitive data (API keys, tokens, passwords, client secrets) and confirmed they use type: 'password'

New Destination Checklist

• Extracted all action API versions to verioning-info.ts file. example

[Copilot]

Pull request overview

This PR adds validation to Klaviyo actions to ensure external_id values do not exceed 255 characters, aligning payload validation with Klaviyo constraints and preventing invalid identifiers from being sent downstream.

Changes:

• Introduces MAX_EXTERNAL_ID_LENGTH = 255 and a shared validateExternalId helper.
• Applies external_id length validation across multiple Klaviyo actions’ perform paths.
• Adds external_id length checks to several batch/multi-status validation helpers in functions.ts.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
packages/destination-actions/src/destinations/klaviyo/upsertProfile/index.ts	Calls validateExternalId during single-event validation before building the profile payload.
packages/destination-actions/src/destinations/klaviyo/trackEvent/index.ts	Adds validateExternalId to reject oversize external_id values for track events.
packages/destination-actions/src/destinations/klaviyo/removeProfileFromList/index.ts	Adds validateExternalId prior to profile lookup/removal.
packages/destination-actions/src/destinations/klaviyo/removeProfile/index.ts	Adds validateExternalId before attempting profile removal.
packages/destination-actions/src/destinations/klaviyo/orderCompleted/index.ts	Adds validateExternalId validation for order-completed events.
packages/destination-actions/src/destinations/klaviyo/functions.ts	Adds validateExternalId, uses it in createProfile, and adds max-length checks in multiple validation helpers.
packages/destination-actions/src/destinations/klaviyo/config.ts	Defines the shared constant MAX_EXTERNAL_ID_LENGTH.

[codecov]

Codecov Report

❌ Patch coverage is 67.85714% with 9 lines in your changes missing coverage. Please review.
✅ Project coverage is 80.95%. Comparing base (139ddfe) to head (ca65e65).

Files with missing lines	Patch %	Lines
...tion-actions/src/destinations/klaviyo/functions.ts	52.63%	6 Missing and 3 partials ⚠️

❌ Your patch check has failed because the patch coverage (67.85%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3777      +/-   ##
==========================================
+ Coverage   80.84%   80.95%   +0.10%     
==========================================
  Files        1377     1351      -26     
  Lines       26417    25325    -1092     
  Branches     5581     5277     -304     
==========================================
- Hits        21357    20501     -856     
+ Misses       4097     3864     -233     
+ Partials      963      960       -3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Copilot]

Pull request overview

Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.

[arnav777dev]

Code Review Personae Verdict: Changes Required ❌

3 passes found 2 critical bugs affecting the addProfileToList and removeProfileFromList actions: (1) validateExternalId is placed inside the try block in createProfile, causing PayloadValidationError to be silently swallowed — the action proceeds with profileId=undefined and sends a malformed request to Klaviyo; (2) minimum/maximum on a string type field in properties.ts uses wrong JSON Schema constraint types, producing AggregateAjvError instead of the intended PayloadValidationError for removeProfileFromList.

Warning

Required Actions

1. Move validateExternalId outside try block in createProfile (functions.ts:111)
2. Fix minimum/maximum to correct string constraints in properties.ts:33
3. Add validateExternalId call to removeProfile/index.ts perform path
4. Fix removeProfileFromList test to expect PayloadValidationError not AggregateAjvError

---

📝 Code Review Results (3 Passes) — 2 Critical, 3 High, 3 Medium ❌

All 3 passes independently identified the same core defects. The removeProfileFromList test expecting AggregateAjvError (while all other new tests expect a specific message) is a consistent cross-pass signal confirming the schema constraint approach is broken. Pass 2 also identified the missing validateExternalId in removeProfile single-event perform.

Findings

• 🚨 CRITICAL: validateExternalId inside try block — error silently swallowed (packages/destination-actions/src/destinations/klaviyo/functions.ts:111)

  [Found by 3/3 passes] validateExternalId(external_id) is placed inside the try block of createProfile. The surrounding catch block catches ALL exceptions from within the try and handles them as KlaviyoAPIError. When validateExternalId throws a PayloadValidationError, the catch handler finds response is undefined (since PayloadValidationError has no .response property), the 409 check is false, and the catch block exits — createProfile returns undefined. The caller then proceeds with profileId = undefined, building { data: [{ type: 'profile', id: undefined }] } and sending a malformed request to Klaviyo. The validation error is completely swallowed — no rejection, just a silent malformed API call.

  Lifecycle trace: validateExternalId throws PayloadValidationError → caught as KlaviyoAPIError → response is undefined → 409 check fails → function returns undefined → addProfileToList(request, undefined, list_id) is called.

  Fix: Move validateExternalId(external_id) to BEFORE the try block.

• 🚨 CRITICAL: minimum/maximum are numeric constraints on a string field (packages/destination-actions/src/destinations/klaviyo/properties.ts:33)

  [Found by 3/3 passes] minimum: 0 and maximum: 255 are JSON Schema numeric constraints. For type: 'string' fields, length is controlled by minLength and maxLength. The framework's fields-to-jsonschema.ts uses if (minimum) (truthiness check) — minimum: 0 is falsy so minLength is never set. The maximum: 255 applied to a string field causes AJV to throw an AggregateAjvError rather than the intended PayloadValidationError. Confirmed by the removeProfileFromList test which expects AggregateAjvError while every other parallel test in this PR expects a specific message string.

  Fix: Remove minimum and maximum and rely solely on the runtime validateExternalId checks.

• 🔴 HIGH: validateExternalId missing in removeProfile single-event perform path (packages/destination-actions/src/destinations/klaviyo/removeProfile/index.ts:64)

  [Found by 2/3 passes] The diff adds a test calling testAction('removeProfile', ...) (single-event perform path) expecting a PayloadValidationError. However, removeProfile/index.ts perform does NOT call validateExternalId. Only the batch path (through validateAndConstructRemoveProfilePayloads) has the check.

  Fix: Import and call validateExternalId(external_id) in removeProfile/index.ts perform, after the required-field check.

• 🔴 HIGH: external_id check after phone mutation in validateAndPreparePayloads (packages/destination-actions/src/destinations/klaviyo/functions.ts:797)

  [Found by 2/3 passes] In validateAndPreparePayloads (batch trackEvent path), the new external_id check is placed AFTER the phone_number processing block that mutates payload.profile.phone_number and deletes payload.profile.country_code. If a payload has both an invalid phone number AND an external_id > 255, only the phone number error is reported. When external_id is the only issue, the error response's sent data contains the partially-mutated payload.

  Fix: Move the external_id check to before the phone number validation block.

• 🟡 MEDIUM: Test expects AggregateAjvError not PayloadValidationError — inconsistent (packages/destination-actions/src/destinations/klaviyo/removeProfileFromList/__tests__/index.test.ts:38)

  [Found by 3/3 passes] Every other test in this PR expects .rejects.toThrowError('Length of external_id must be no more than 255 characters.'). Only this test expects AggregateAjvError. After fixing properties.ts, this test should be updated to match.

• 🟡 MEDIUM: minimum: 0 is falsy — silently skips minLength enforcement (packages/destination-actions/src/destinations/klaviyo/properties.ts:35)

  [Found by 2/3 passes] if (minimum) in fields-to-jsonschema.ts is a truthiness check. minimum: 0 evaluates as false, so minLength is never set. Remove minimum: 0 entirely.

_{Generated by code-review-personae v0.7.0 | Deep Code Review (3 Passes)}

[arnav777dev]

Caution

Bug: minimum/maximum are numeric JSON Schema constraints — wrong type for a string field

minimum and maximum apply to numbers. For strings, use minLength / maxLength.

The framework's fields-to-jsonschema.ts checks if (minimum) (truthiness). Since minimum: 0 is falsy, minLength is never set — it silently does nothing. The maximum: 255 applied to a string field causes AJV to throw an opaque AggregateAjvError instead of the intended PayloadValidationError. This is confirmed by the removeProfileFromList test which expects AggregateAjvError — while every other test in this PR expects the friendly message string.

Fix: Remove minimum and maximum entirely and rely on the runtime validateExternalId checks that are already in place:

export const external_id: InputField = {
  label: 'External ID',
  description: `A unique identifier used by customers to associate Klaviyo profiles with profiles in an external system. One of External ID and Email required. Must not exceed 255 characters.`,
  type: 'string'
  // 255-char limit enforced at runtime by validateExternalId()
}

[AnkitSegment]

The framework's fields-to-jsonschema.ts actually handles this correctly for string types — it converts minimum → minLength and maximum → maxLength when field.type === 'string'. So maximum: 255 does enforce a 255-character limit at the schema level.

The minimum: 0 being falsy is harmless since minLength: 0 is meaningless anyway.

Keeping minimum/maximum in place since it provides schema-level validation before perform is ever called. The runtime validateExternalId serves as a safety net for batch paths.

Updated the description to mention Phone Number as a valid identifier alternative.

[arnav777dev]

Warning

Bug: external_id length check placed after phone_number mutation

This check fires after the block above (lines 782–797) has already:

1. Validated phone_number
2. Mutated payload.profile.phone_number with the E.164 form
3. Deleted payload.profile.country_code

If a payload has an invalid phone number and an external_id > 255 chars, only the phone error is reported and the function returns early — the external_id violation is silently missed. The caller has to make a second attempt (after fixing the phone number) to discover the external_id problem.

More critically: when external_id is the only issue, the payload object has already been mutated before this early return. The sent field in the error response will contain the partially-mutated payload (E.164 phone, no country_code) rather than the original event data.

Fix: Move this check to immediately after the all-identifiers-missing check (before the phone_number block), consistent with how validateAndConstructRemoveProfilePayloads and validateAndConstructProfilePayload order their validations:

if (!email && !phone_number && !external_id && !anonymous_id) { ... return }

// Move here ↓
if (external_id && external_id.length > MAX_EXTERNAL_ID_LENGTH) {
  multiStatusResponse.setErrorResponseAtIndex(originalBatchIndex, EXTERNAL_ID_LENGTH_ERROR)
  return
}

if (phone_number) { ... }  // mutations happen after all pre-checks

[AnkitSegment]

Fixed. Moved the external_id length check to before the phone_number validation/mutation block in validateAndPreparePayloads. Now the order is:

1. Check all identifiers missing
2. Check external_id length
3. Validate/mutate phone_number

This ensures no payload mutation happens before early-return on external_id error, and both issues are reported independently.

[arnav777dev]

Warning

Inconsistency: this test expects AggregateAjvError but all other equivalent tests in this PR expect a specific message

Every other new external_id test added in this PR (removeProfile, trackEvent, orderCompleted, upsertProfile) expects:

.rejects.toThrowError('Length of external_id must be no more than 255 characters.')

This test expects AggregateAjvError — which is the AJV schema-level validator firing before the custom validateExternalId runtime check is ever reached. This is a symptom of the bug in properties.ts where maximum: 255 is applied to a type: 'string' field using the wrong constraint type.

The user sees an opaque schema error rather than the intended descriptive message.

Fix: After removing minimum/maximum from properties.ts, change this to match the other tests:

await expect(
  testDestination.testAction('removeProfileFromList', { event, mapping, settings })
).rejects.toThrowError('Length of external_id must be no more than 255 characters.')

[AnkitSegment]

The AggregateAjvError expectation here is intentional and correct. The framework's fields-to-jsonschema.ts converts maximum: 255 on string-type fields to maxLength: 255 in the JSON Schema. This means schema validation catches the >255 char external_id before the perform method is called, resulting in AggregateAjvError.

This is consistent with how removeProfileFromList uses the shared external_id field from properties.ts with maximum: 255 — schema validation fires first. The other tests (trackEvent, orderCompleted, etc.) use validateExternalId in their perform path because their field definitions don't all carry the schema constraint directly.

Both approaches reject invalid input — schema-level is just earlier in the pipeline.

[Copilot]

Pull request overview

Copilot reviewed 16 out of 16 changed files in this pull request and generated 3 comments.

[arnav777dev]