Conversation
🦋 Changeset detectedLatest commit: 83adb2b The changes in this PR will be included in the next version bump. This PR includes changesets to release 0 packagesWhen changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughAdds a new KYC migration script that batch-processes unknown accounts, fetches approved inquiries (template-aware), updates or redacts accounts via new Persona helpers, extends Persona types/schemas, updates tests/fixtures, and exports a validator helper for message formatting. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Script as KYC Migration Script
participant PersonaAPI as Persona API
participant DB as Database
participant Processor as Account Processor
User->>Script: run with CLI options
Script->>PersonaAPI: getUnknownAccounts(batchSize, after?, referenceId?)
PersonaAPI->>DB: query unknown accounts (pagination)
DB-->>PersonaAPI: accounts[]
PersonaAPI-->>Script: accounts[]
loop per account
Script->>Processor: processAccount(account)
Processor->>PersonaAPI: getUnknownApprovedInquiry(referenceId, templateId?)
PersonaAPI->>DB: fetch approved inquiry
DB-->>PersonaAPI: inquiry or null
PersonaAPI-->>Processor: inquiry
alt inquiry exists and not redacted
Processor->>Processor: parse template (Panda / Cryptomate / unknown)
alt Panda template
Processor->>PersonaAPI: updateAccount(accountId, attributes)
PersonaAPI->>DB: patch account, add document
DB-->>PersonaAPI: success
else Cryptomate or unknown
Processor->>Processor: log/skip or mark failure
end
else inquiry missing or redacted
Processor->>PersonaAPI: redactAccount(accountId)
PersonaAPI->>DB: patch redaction
DB-->>PersonaAPI: success
end
Processor-->>Script: account result (migrated / redacted / failed)
end
Script->>Script: compile statistics
Script-->>User: print migration summary
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
Summary of ChangesHello @mainqueg, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new server-side script aimed at migrating Know Your Customer (KYC) account data. The primary goal is to automate the process of updating account information and associated documents by leveraging approved Persona inquiries. It includes mechanisms for identifying and processing specific inquiry templates, redacting non-compliant accounts, and ensuring data consistency through robust validation. This change significantly enhances the system's ability to manage and maintain accurate KYC records. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new script, kyc-migration.ts, to migrate and redact KYC accounts. The review suggests removing an unnecessary eslint disable comment and emphasizes throwing errors for schema validation issues to prevent data corruption, aligning with data integrity best practices. It also modifies the persona utilities, where a comment highlights the importance of avoiding direct object mutation and demonstrates using placeholders with documentation for unavailable required fields, consistent with data adaptation guidelines.
| unexpected( | ||
| `❌ Account ${referenceId}/${accountId} failed to parse panda inquiry`, | ||
| buildIssueMessages(pandaInquiry.issues), | ||
| ); |
There was a problem hiding this comment.
Consider throwing an error here, instead of just returning. This will ensure that the migration process stops when there are schema errors, which can prevent data corruption. This aligns with the principle of treating data integrity issues as critical, similar to how missing expected database records should result in an error.
References
- Throw an error when an expected database record (like a card) is not found. This indicates a system or data integrity issue, not a client-side error that can be fixed by the user, so a 5xx-level error is more appropriate than a 4xx.
There was a problem hiding this comment.
The objective is to migrate all possible accounts and flag problematic ones for separate handling. Since the migration is idempotent, it can be rerun safely without side effects. Given the volume of accounts, stopping the migration for individual errors isn’t practical
| let next = initialNext; | ||
| let batch = 0; | ||
| let retries = 0; | ||
| // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition |
There was a problem hiding this comment.
The eslint disable comment is unnecessary because the condition can be while (true) without any issues.
while (true) {There was a problem hiding this comment.
Unnecessary conditional, value is always truthy.eslint@typescript-eslint/no-unnecessary-condition
| transform((fields) => { | ||
| if (!fields["new-screen-2-2-input-checkbox"]?.value && !fields["new-screen-input-checkbox-2"]?.value) { | ||
| // eslint-disable-next-line no-console | ||
| console.error( | ||
| "❌ exa card tc is required, either new-screen-2-2-input-checkbox or new-screen-input-checkbox-2 must be true, setting new-screen-input-checkbox-2 to true", | ||
| ); | ||
| fields["new-screen-input-checkbox-2"] = { value: true }; | ||
| return fields; | ||
| } | ||
| return fields; | ||
| }), |
There was a problem hiding this comment.
This transform function modifies the fields object directly and mutates the new-screen-input-checkbox-2 property. This could lead to unexpected side effects if the original object is used elsewhere. It's better to create a new object with the modified property to avoid mutation. Additionally, the suggested code demonstrates how to use a reasonable placeholder ({ value: true }) for a required field when it's unavailable, and documents this substitution with a console.error, aligning with best practices for adapting data formats.
transform((fields) => {
if (!fields["new-screen-2-2-input-checkbox"]?.value && !fields["new-screen-input-checkbox-2"]?.value) {
// eslint-disable-next-line no-console
console.error(
"❌ exa card tc is required, either new-screen-2-2-input-checkbox or new-screen-input-checkbox-2 must be true, setting new-screen-input-checkbox-2 to true",
);
return { ...fields, ["new-screen-input-checkbox-2"]: { value: true } }; // Create a new object instead of mutating the original
}
return fields;
}),References
- When adapting data formats, if a required field is unavailable in the source, use a reasonable placeholder from the available data to satisfy the format and document this substitution with a code comment.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #668 +/- ##
==========================================
- Coverage 63.66% 61.07% -2.59%
==========================================
Files 169 169
Lines 5882 5172 -710
Branches 1753 1449 -304
==========================================
- Hits 3745 3159 -586
+ Misses 1964 1858 -106
+ Partials 173 155 -18
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🤖 Fix all issues with AI agents
In `@server/script/kyc-migration.ts`:
- Around line 201-239: Extract the duplicated redaction block into a helper
function (e.g. redactAccountWithLogging) that accepts accountId and referenceId
(and optionally a reason string) and encapsulates the logic that logs the
reason, checks onlyLogs, calls persona.redactAccount(...), increments
redactedAccounts on success and failedToRedactAccounts on failure, and uses
log/unexpected/inspect for messages; then replace both duplicated sections with
calls to redactAccountWithLogging(referenceId, accountId, "Account ... has no
approved inquiry" or "Inquiry ... is redacted") to remove duplication while
preserving behavior.
- Around line 50-62: Replace the many standalone counters (migratedAccounts,
redactedAccounts, redactedInquiries, failedToRedactAccounts,
noApprovedInquiryAccounts, unknownTemplates, cryptomateTemplates,
pandaTemplates, schemaErrors, failedAccounts, inquirySchemaErrors,
noReferenceIdAccounts, totalAccounts) with a single statistics object (e.g.,
const statistics = { migrated: 0, redactedAccounts: 0, redactedInquiries: 0,
failedToRedact: 0, noApprovedInquiry: 0, unknownTemplates: 0,
cryptomateTemplates: 0, pandaTemplates: 0, schemaErrors: 0, failed: 0,
inquirySchemaErrors: 0, noReferenceId: 0, total: 0 };), then update every place
that reads or mutates the old variables (e.g., migratedAccounts++, totalAccounts
+= n, logging output, resets) to use the corresponding statistics.<property>
(e.g., statistics.migrated++, statistics.total += n) and add a small TypeScript
interface/type for the object if needed; ensure all logging and tests reference
the object and that any reset logic sets statistics = { ...0s } or zeroes each
property.
- Around line 117-125: The retry delay in the catch block of kyc-migration.ts is
ineffective because Promise.resolve(setTimeout(...)) resolves immediately;
replace that line in the catch block (the code around unexpected(...),
retries++, and the retry logic) with an actual awaited delay by awaiting a
Promise that resolves after 1000ms (i.e., create a new Promise and resolve it
from setTimeout, then await it) so the retry truly waits one second before
continuing. Ensure the surrounding async context (the catch handler) remains
unchanged and keep retries and the abort logic intact.
- Around line 21-44: The option parsing silently allows missing values for flags
parsed with option.split("=")[1] (e.g., setting reference or initialNext to
undefined); update the switch cases that handle "--reference-id=" and "--next="
to validate that option includes "=" and a non-empty value before assigning to
reference or initialNext, and if the value is missing call unexpected(...) or
throw an Error with a clear message; ensure you check both the existence and
non-empty string (trimmed) of the right-hand part and apply the same validation
pattern wherever option.split("=")[1] is used.
- Around line 15-19: The module-level mutable variables reference, all,
onlyLogs, initialNext, and onlyPandaTemplates should not be declared as let;
instead create a single immutable options object (e.g., const migrationOptions =
{ reference: undefined, all: false, onlyLogs: false, initialNext: undefined,
onlyPandaTemplates: false }) or require these values as parameters to main(),
then read/update only within a local scope; update usages where the code
currently reads/writes reference, all, onlyLogs, initialNext, and
onlyPandaTemplates to access properties on the options object or the function
parameters and remove the module-level lets.
- Around line 266-287: The catch for persona.getAccount currently swallows all
errors so non-schema failures (e.g., network) are treated as schemaErrors;
modify the catch in the getAccount call so that if error instanceof ValiError
you call unexpected(...) and allow basicAccount to remain falsy, but for any
other error re-throw the error (or propagate it) instead of swallowing it;
reference persona.getAccount, ValiError, basicAccount, schemaErrors and
unexpected when making the change so non-schema errors aren't misclassified.
In `@server/utils/persona.ts`:
- Around line 59-65: The request URL in getUnknownApprovedInquiry currently
appends `&${templateId ? \`filter[inquiry-template-id]=${templateId}\` : ""}`
which leaves a trailing ampersand when templateId is undefined; fix by
constructing the query string from an array of parameters (e.g., base params
"page[size]=1", "filter[reference-id]=${referenceId}",
"filter[status]=approved") and only push
`filter[inquiry-template-id]=${templateId}` when templateId is provided, then
join with '&' and pass the resulting clean query string into the request call to
eliminate the stray '&'.
- Around line 534-544: Replace the console.error call and in-place mutation
inside the transform callback with structured logging and an immutable update:
inside the transform((fields) => { ... }) callback (the function that checks
"new-screen-2-2-input-checkbox" and "new-screen-input-checkbox-2"), remove the
console.error and instead call the debug/logger provided via the transform
context (e.g., use the passed-in debug/logger on the transform context or
closure) to log a structured message including the field names and reason, and
when setting new-screen-input-checkbox-2 to true return a new object (e.g., {
...fields, "new-screen-input-checkbox-2": { value: true } }) rather than
mutating fields in place.
| let reference: string | undefined; | ||
| let all = false; | ||
| let onlyLogs = false; | ||
| let initialNext: string | undefined; | ||
| let onlyPandaTemplates = false; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Prefer const with explicit initialization for module-level state.
Per coding guidelines, prefer const for all variable declarations. These mutable module-level variables could cause issues if the script is imported multiple times or used in tests.
Consider encapsulating these in an options object or passing them as parameters to main():
Proposed approach
-let reference: string | undefined;
-let all = false;
-let onlyLogs = false;
-let initialNext: string | undefined;
-let onlyPandaTemplates = false;
+interface MigrationOptions {
+ reference?: string;
+ all: boolean;
+ onlyLogs: boolean;
+ initialNext?: string;
+ onlyPandaTemplates: boolean;
+}
+
+function parseOptions(argv: string[]): MigrationOptions {
+ const options: MigrationOptions = {
+ all: false,
+ onlyLogs: false,
+ onlyPandaTemplates: false,
+ };
+ // ... parsing logic
+ return options;
+}🤖 Prompt for AI Agents
In `@server/script/kyc-migration.ts` around lines 15 - 19, The module-level
mutable variables reference, all, onlyLogs, initialNext, and onlyPandaTemplates
should not be declared as let; instead create a single immutable options object
(e.g., const migrationOptions = { reference: undefined, all: false, onlyLogs:
false, initialNext: undefined, onlyPandaTemplates: false }) or require these
values as parameters to main(), then read/update only within a local scope;
update usages where the code currently reads/writes reference, all, onlyLogs,
initialNext, and onlyPandaTemplates to access properties on the options object
or the function parameters and remove the module-level lets.
| const options = process.argv.slice(2); | ||
| for (const option of options) { | ||
| switch (true) { | ||
| case option.startsWith("--reference-id="): | ||
| reference = option.split("=")[1]; | ||
| break; | ||
| case option.startsWith("--all"): | ||
| all = true; | ||
| break; | ||
| case option.startsWith("--only-logs"): | ||
| log("Running in only logs mode"); | ||
| onlyLogs = true; | ||
| break; | ||
| case option.startsWith("--next="): | ||
| initialNext = option.split("=")[1]; | ||
| break; | ||
| case option.startsWith("--only-panda-templates"): | ||
| onlyPandaTemplates = true; | ||
| break; | ||
| default: | ||
| unexpected(`❌ unknown option: ${option}`); | ||
| throw new Error(`unknown option: ${option}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
Missing validation for option value extraction.
option.split("=")[1] returns undefined if the = is missing or value is empty (e.g., --reference-id=), which silently sets reference to undefined.
Proposed fix
case option.startsWith("--reference-id="):
- reference = option.split("=")[1];
+ reference = option.split("=")[1];
+ if (!reference) {
+ unexpected("❌ --reference-id requires a value");
+ throw new Error("--reference-id requires a value");
+ }
break;🤖 Prompt for AI Agents
In `@server/script/kyc-migration.ts` around lines 21 - 44, The option parsing
silently allows missing values for flags parsed with option.split("=")[1] (e.g.,
setting reference or initialNext to undefined); update the switch cases that
handle "--reference-id=" and "--next=" to validate that option includes "=" and
a non-empty value before assigning to reference or initialNext, and if the value
is missing call unexpected(...) or throw an Error with a clear message; ensure
you check both the existence and non-empty string (trimmed) of the right-hand
part and apply the same validation pattern wherever option.split("=")[1] is
used.
| let migratedAccounts = 0; | ||
| let redactedAccounts = 0; | ||
| let redactedInquiries = 0; | ||
| let failedToRedactAccounts = 0; | ||
| let noApprovedInquiryAccounts = 0; | ||
| let unknownTemplates = 0; | ||
| let cryptomateTemplates = 0; | ||
| let pandaTemplates = 0; | ||
| let schemaErrors = 0; | ||
| let failedAccounts = 0; | ||
| let inquirySchemaErrors = 0; | ||
| let noReferenceIdAccounts = 0; | ||
| let totalAccounts = 0; |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider encapsulating statistics in an object.
Multiple global counters could be grouped into a single statistics object for cleaner code and easier reset/testing.
Proposed approach
const statistics = {
migrated: 0,
redactedAccounts: 0,
redactedInquiries: 0,
failedToRedact: 0,
noApprovedInquiry: 0,
unknownTemplates: 0,
cryptomateTemplates: 0,
pandaTemplates: 0,
schemaErrors: 0,
failed: 0,
inquirySchemaErrors: 0,
noReferenceId: 0,
total: 0,
};🤖 Prompt for AI Agents
In `@server/script/kyc-migration.ts` around lines 50 - 62, Replace the many
standalone counters (migratedAccounts, redactedAccounts, redactedInquiries,
failedToRedactAccounts, noApprovedInquiryAccounts, unknownTemplates,
cryptomateTemplates, pandaTemplates, schemaErrors, failedAccounts,
inquirySchemaErrors, noReferenceIdAccounts, totalAccounts) with a single
statistics object (e.g., const statistics = { migrated: 0, redactedAccounts: 0,
redactedInquiries: 0, failedToRedact: 0, noApprovedInquiry: 0, unknownTemplates:
0, cryptomateTemplates: 0, pandaTemplates: 0, schemaErrors: 0, failed: 0,
inquirySchemaErrors: 0, noReferenceId: 0, total: 0 };), then update every place
that reads or mutates the old variables (e.g., migratedAccounts++, totalAccounts
+= n, logging output, resets) to use the corresponding statistics.<property>
(e.g., statistics.migrated++, statistics.total += n) and add a small TypeScript
interface/type for the object if needed; ensure all logging and tests reference
the object and that any reset logic sets statistics = { ...0s } or zeroes each
property.
| if (!unknownInquiry) { | ||
| noApprovedInquiryAccounts++; | ||
| log(`Account ${referenceId}/${accountId} has no approved inquiry. Redacting account...`); | ||
| if (onlyLogs) return; | ||
| await persona | ||
| .redactAccount(accountId) | ||
| .then(() => { | ||
| log(`♻️ Account ${referenceId}/${accountId} redacted successfully`); | ||
| redactedAccounts++; | ||
| }) | ||
| .catch((error: unknown) => { | ||
| unexpected( | ||
| `❌ Account ${referenceId}/${accountId} redacting failed`, | ||
| inspect(error, { depth: null, colors: true }), | ||
| ); | ||
| failedToRedactAccounts++; | ||
| }); | ||
| return; | ||
| } | ||
|
|
||
| if (unknownInquiry.attributes["redacted-at"]) { | ||
| redactedInquiries++; | ||
| log(`Inquiry ${referenceId}/${accountId} is redacted. Redacting account...`); | ||
| if (onlyLogs) return; | ||
| await persona | ||
| .redactAccount(accountId) | ||
| .then(() => { | ||
| log(`♻️ Account ${referenceId}/${accountId} redacted successfully`); | ||
| redactedAccounts++; | ||
| }) | ||
| .catch((error: unknown) => { | ||
| unexpected( | ||
| `❌ Account ${referenceId}/${accountId} redacting failed`, | ||
| inspect(error, { depth: null, colors: true }), | ||
| ); | ||
| failedToRedactAccounts++; | ||
| }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Duplicate redaction logic could be extracted.
The redaction code in lines 205-218 and 225-238 is nearly identical. Consider extracting to a helper function.
Proposed approach
async function redactAccountWithLogging(accountId: string, referenceId: string, reason: string) {
log(`${reason}. Redacting account...`);
if (onlyLogs) return;
await persona
.redactAccount(accountId)
.then(() => {
log(`♻️ Account ${referenceId}/${accountId} redacted successfully`);
redactedAccounts++;
})
.catch((error: unknown) => {
unexpected(
`❌ Account ${referenceId}/${accountId} redacting failed`,
inspect(error, { depth: null, colors: true }),
);
failedToRedactAccounts++;
});
}🤖 Prompt for AI Agents
In `@server/script/kyc-migration.ts` around lines 201 - 239, Extract the
duplicated redaction block into a helper function (e.g.
redactAccountWithLogging) that accepts accountId and referenceId (and optionally
a reason string) and encapsulates the logic that logs the reason, checks
onlyLogs, calls persona.redactAccount(...), increments redactedAccounts on
success and failedToRedactAccounts on failure, and uses log/unexpected/inspect
for messages; then replace both duplicated sections with calls to
redactAccountWithLogging(referenceId, accountId, "Account ... has no approved
inquiry" or "Inquiry ... is redacted") to remove duplication while preserving
behavior.
| // validate basic scope | ||
| const basicAccount = await persona.getAccount(referenceId, "basic").catch((error: unknown) => { | ||
| if (error instanceof ValiError) { | ||
| unexpected( | ||
| `❌ Account ${referenceId}/${accountId} failed to get basic scope due to schema errors`, | ||
| buildIssueMessages(error.issues), | ||
| ); | ||
| } else { | ||
| unexpected( | ||
| `❌ Account ${referenceId}/${accountId} getting basic scope failed`, | ||
| inspect(error, { depth: null, colors: true }), | ||
| ); | ||
| } | ||
| }); | ||
|
|
||
| if (!basicAccount) { | ||
| schemaErrors++; | ||
| return unexpected(`❌ Account ${referenceId}/${accountId} failed to get basic scope`); | ||
| } | ||
| log(`✅ PANDA TEMPLATE: Account ${referenceId}/${basicAccount.id} has been migrated and has a valid basic scope`); | ||
| migratedAccounts++; | ||
| return; |
There was a problem hiding this comment.
Error handling in getAccount catch block may not correctly track failures.
The catch block logs errors but doesn't re-throw. The subsequent check if (!basicAccount) will be true, incrementing schemaErrors even for non-schema errors (like network failures).
Proposed fix
const basicAccount = await persona.getAccount(referenceId, "basic").catch((error: unknown) => {
if (error instanceof ValiError) {
+ schemaErrors++;
unexpected(
`❌ Account ${referenceId}/${accountId} failed to get basic scope due to schema errors`,
buildIssueMessages(error.issues),
);
} else {
+ failedAccounts++;
unexpected(
`❌ Account ${referenceId}/${accountId} getting basic scope failed`,
inspect(error, { depth: null, colors: true }),
);
}
+ return undefined;
});
if (!basicAccount) {
- schemaErrors++;
return unexpected(`❌ Account ${referenceId}/${accountId} failed to get basic scope`);
}🤖 Prompt for AI Agents
In `@server/script/kyc-migration.ts` around lines 266 - 287, The catch for
persona.getAccount currently swallows all errors so non-schema failures (e.g.,
network) are treated as schemaErrors; modify the catch in the getAccount call so
that if error instanceof ValiError you call unexpected(...) and allow
basicAccount to remain falsy, but for any other error re-throw the error (or
propagate it) instead of swallowing it; reference persona.getAccount, ValiError,
basicAccount, schemaErrors and unexpected when making the change so non-schema
errors aren't misclassified.
| transform((fields) => { | ||
| if (!fields["new-screen-2-2-input-checkbox"]?.value && !fields["new-screen-input-checkbox-2"]?.value) { | ||
| // eslint-disable-next-line no-console | ||
| console.error( | ||
| "❌ exa card tc is required, either new-screen-2-2-input-checkbox or new-screen-input-checkbox-2 must be true, setting new-screen-input-checkbox-2 to true", | ||
| ); | ||
| fields["new-screen-input-checkbox-2"] = { value: true }; | ||
| return fields; | ||
| } | ||
| return fields; | ||
| }), |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Avoid console.error in schema transforms; use structured logging.
Schema transforms should not use console.error directly. This bypasses the structured logging setup and may not appear in proper log streams. Consider removing the log or using the debug logger passed through context.
Additionally, the transform mutates the input object directly. While this works, returning a new object would be cleaner.
Proposed fix
transform((fields) => {
if (!fields["new-screen-2-2-input-checkbox"]?.value && !fields["new-screen-input-checkbox-2"]?.value) {
- // eslint-disable-next-line no-console
- console.error(
- "❌ exa card tc is required, either new-screen-2-2-input-checkbox or new-screen-input-checkbox-2 must be true, setting new-screen-input-checkbox-2 to true",
- );
- fields["new-screen-input-checkbox-2"] = { value: true };
- return fields;
+ // fallback to true when both checkbox fields are missing
+ return { ...fields, "new-screen-input-checkbox-2": { value: true } };
}
return fields;
}),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| transform((fields) => { | |
| if (!fields["new-screen-2-2-input-checkbox"]?.value && !fields["new-screen-input-checkbox-2"]?.value) { | |
| // eslint-disable-next-line no-console | |
| console.error( | |
| "❌ exa card tc is required, either new-screen-2-2-input-checkbox or new-screen-input-checkbox-2 must be true, setting new-screen-input-checkbox-2 to true", | |
| ); | |
| fields["new-screen-input-checkbox-2"] = { value: true }; | |
| return fields; | |
| } | |
| return fields; | |
| }), | |
| transform((fields) => { | |
| if (!fields["new-screen-2-2-input-checkbox"]?.value && !fields["new-screen-input-checkbox-2"]?.value) { | |
| // fallback to true when both checkbox fields are missing | |
| return { ...fields, "new-screen-input-checkbox-2": { value: true } }; | |
| } | |
| return fields; | |
| }), |
🤖 Prompt for AI Agents
In `@server/utils/persona.ts` around lines 534 - 544, Replace the console.error
call and in-place mutation inside the transform callback with structured logging
and an immutable update: inside the transform((fields) => { ... }) callback (the
function that checks "new-screen-2-2-input-checkbox" and
"new-screen-input-checkbox-2"), remove the console.error and instead call the
debug/logger provided via the transform context (e.g., use the passed-in
debug/logger on the transform context or closure) to log a structured message
including the field names and reason, and when setting
new-screen-input-checkbox-2 to true return a new object (e.g., { ...fields,
"new-screen-input-checkbox-2": { value: true } }) rather than mutating fields in
place.
|
@devin-ai-integration review |
cruzdanilo
force-pushed
the
main
branch
10 times, most recently
from
c7829dc to
5e7861d
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (3)
server/utils/persona.ts (1)
563-572:console.errorin schema transform — use structured logging.This was flagged in a prior review. Schema transforms should not use
console.error. Consider removing the log or using a debug logger. Also, returning a new object (as done on line 569) is good — the mutation issue from the prior review is already addressed.server/script/kyc-migration.ts (2)
320-358: 🧹 Nitpick | 🔵 TrivialDuplicate redaction logic — extract a helper.
The redaction blocks at lines 322-337 and 342-357 are nearly identical (log, guard on
onlyLogs, callredactAccount, increment counters). This was noted in a prior review. Consider extracting aredactAccountWithLogging(accountId, referenceId, reason)helper.
285-301:⚠️ Potential issue | 🟠 MajorInconsistent null handling:
street_2is coerced to""butsubdivisionpassesnullthrough.Line 288 / 297:
street_2/"address-street-2"use?? ""to convert null to empty string.
Line 290 / 299:subdivision/"address-subdivision"pass the nullable value as-is.If the intent is to send empty strings for absent address components, apply the same
?? ""to subdivision. Otherwise,nullsubdivisions will breakgetAccount(_, "basic")(see the related comment onUpdateAccountAttributesinpersona.ts).Option A: coerce subdivision to empty string (matches street_2 treatment)
- subdivision: inquiry.attributes.fields["address-subdivision"].value, + subdivision: inquiry.attributes.fields["address-subdivision"].value ?? "", postal_code: inquiry.attributes.fields["address-postal-code"].value,- "address-subdivision": inquiry.attributes.fields["address-subdivision"].value, + "address-subdivision": inquiry.attributes.fields["address-subdivision"].value ?? "", "address-postal-code": inquiry.attributes.fields["address-postal-code"].value,
| async function loadEntriesFromCsv(csvPath: string): Promise<CsvEntry[]> { | ||
| const entries: CsvEntry[] = []; | ||
| // eslint-disable-next-line security/detect-non-literal-fs-filename | ||
| const fileStream = createReadStream(csvPath); | ||
| const rl = createInterface({ input: fileStream, crlfDelay: Number.POSITIVE_INFINITY }); | ||
|
|
||
| let isHeader = true; | ||
| for await (const line of rl) { | ||
| if (isHeader) { | ||
| isHeader = false; | ||
| continue; | ||
| } | ||
| // parse csv line: "referenceId","accountId","status" | ||
| const match = /^"([^"]*)","([^"]*)"/.exec(line); | ||
| if (match?.[1] && match[2]) { | ||
| entries.push({ referenceId: match[1], accountId: match[2] }); | ||
| } | ||
| } | ||
|
|
||
| return entries; | ||
| } |
There was a problem hiding this comment.
CSV parser is fragile — fields containing escaped quotes or commas will be mis-parsed.
The regex /^"([^"]*)","([^"]*)"/.exec(line) doesn't handle escaped quotes ("") inside fields, nor unquoted fields. If any referenceId or accountId value contains a double-quote character, the match will silently fail and the entry will be skipped without warning.
If the CSV is always machine-generated with safe IDs, this is acceptable. Otherwise, consider using a CSV parsing library.
| main().catch((error: unknown) => { | ||
| unexpected("❌ migration failed", inspect(error, { depth: null, colors: true })); | ||
| }); |
There was a problem hiding this comment.
Process exits with code 0 even when migration fails.
main().catch(…) logs the error but doesn't set process.exitCode = 1 or re-throw, so the script silently reports success to the shell. CI and orchestration tooling won't detect failures.
Proposed fix
main().catch((error: unknown) => {
unexpected("❌ migration failed", inspect(error, { depth: null, colors: true }));
+ process.exitCode = 1;
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| main().catch((error: unknown) => { | |
| unexpected("❌ migration failed", inspect(error, { depth: null, colors: true })); | |
| }); | |
| main().catch((error: unknown) => { | |
| unexpected("❌ migration failed", inspect(error, { depth: null, colors: true })); | |
| process.exitCode = 1; | |
| }); |
| while (true) { | ||
| try { | ||
| log( | ||
| `\n ----- Processing batch ${batch++} (Batch size: ${BATCH_SIZE}, next: ${next ?? "undefined"}) ${retries > 0 ? `(Retry ${retries})` : ""} -----`, | ||
| ); | ||
|
|
||
| const accounts = await getAccounts(BATCH_SIZE, next ?? undefined, reference).catch((error: unknown) => { | ||
| if (error instanceof ValiError) { | ||
| unexpected(`❌ Failed process batch ${batch} due to schema errors. Aborting...`); | ||
| unexpected("❌ Schema errors:", buildIssueMessages(error.issues)); | ||
| return { data: [], links: { next: null } }; | ||
| } | ||
| throw error; | ||
| }); |
There was a problem hiding this comment.
Schema-error catch silently ends the migration without signaling failure.
When getAccounts throws a ValiError (line 141), the catch returns { data: [], links: { next: null } }. This causes next to be undefined on line 179, breaking the loop. The log says "Aborting…" but the script proceeds to printSummary() and exits with code 0, as though it completed normally. There is no indication to the caller (or to CI) that a partial run occurred.
Consider throwing or setting a flag to force a non-zero exit:
Proposed fix
const accounts = await getAccounts(BATCH_SIZE, next ?? undefined, reference).catch((error: unknown) => {
if (error instanceof ValiError) {
unexpected(`❌ Failed process batch ${batch} due to schema errors. Aborting...`);
unexpected("❌ Schema errors:", buildIssueMessages(error.issues));
- return { data: [], links: { next: null } };
+ process.exitCode = 1;
+ return { data: [] as InferOutput<typeof persona.UnknownAccount>["data"], links: { next: null } };
}
throw error;
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| while (true) { | |
| try { | |
| log( | |
| `\n ----- Processing batch ${batch++} (Batch size: ${BATCH_SIZE}, next: ${next ?? "undefined"}) ${retries > 0 ? `(Retry ${retries})` : ""} -----`, | |
| ); | |
| const accounts = await getAccounts(BATCH_SIZE, next ?? undefined, reference).catch((error: unknown) => { | |
| if (error instanceof ValiError) { | |
| unexpected(`❌ Failed process batch ${batch} due to schema errors. Aborting...`); | |
| unexpected("❌ Schema errors:", buildIssueMessages(error.issues)); | |
| return { data: [], links: { next: null } }; | |
| } | |
| throw error; | |
| }); | |
| while (true) { | |
| try { | |
| log( | |
| `\n ----- Processing batch ${batch++} (Batch size: ${BATCH_SIZE}, next: ${next ?? "undefined"}) ${retries > 0 ? `(Retry ${retries})` : ""} -----`, | |
| ); | |
| const accounts = await getAccounts(BATCH_SIZE, next ?? undefined, reference).catch((error: unknown) => { | |
| if (error instanceof ValiError) { | |
| unexpected(`❌ Failed process batch ${batch} due to schema errors. Aborting...`); | |
| unexpected("❌ Schema errors:", buildIssueMessages(error.issues)); | |
| process.exitCode = 1; | |
| return { data: [] as InferOutput<typeof persona.UnknownAccount>["data"], links: { next: null } }; | |
| } | |
| throw error; | |
| }); |
| async function processOnlyFailed(csvPath: string) { | ||
| const entries = await loadEntriesFromCsv(csvPath); | ||
| log(`📋 Loaded ${entries.length} failed accounts from ${csvPath}`); | ||
| log("🔄 Mode: only-failed (processing accounts directly from CSV)"); | ||
|
|
||
| totalAccounts = entries.length; | ||
|
|
||
| for (let index = 0; index < entries.length; index++) { | ||
| const entry = entries[index]; | ||
| if (!entry) continue; | ||
|
|
||
| const { referenceId, accountId } = entry; | ||
|
|
||
| if (index % BATCH_SIZE === 0) { | ||
| log(`\n ----- Processing batch ${Math.floor(index / BATCH_SIZE)} (${index}/${entries.length}) -----`); | ||
| } | ||
|
|
||
| try { | ||
| await processAccount(accountId, referenceId); | ||
| } catch (error: unknown) { | ||
| unexpected( | ||
| `❌ Failed to process account ${accountId}/${referenceId} due to: ${inspect(error, { depth: null, colors: true })}`, | ||
| ); | ||
| failedAccounts++; | ||
| await new Promise((resolve) => setTimeout(resolve, 1000)); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
No rate-limiting or throttling between API calls in processOnlyFailed.
For large CSV files, this loop fires API calls as fast as possible with no delay between successful operations. The batch-processing path at least has natural pagination pauses. Consider adding a small delay or batching to avoid hitting Persona API rate limits.
| function getAccounts(limit: number, after?: string, referenceId?: string) { | ||
| return persona.getUnknownAccounts(limit, after, referenceId); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Single-use wrapper violates coding guidelines.
getAccounts on line 253 is a trivial wrapper around persona.getUnknownAccounts and is only called once (line 140). Per guidelines, single-use logic should not be extracted into a separate function — call persona.getUnknownAccounts directly.
Proposed fix
- const accounts = await getAccounts(BATCH_SIZE, next ?? undefined, reference).catch((error: unknown) => {
+ const accounts = await persona.getUnknownAccounts(BATCH_SIZE, next ?? undefined, reference).catch((error: unknown) => {and remove:
-function getAccounts(limit: number, after?: string, referenceId?: string) {
- return persona.getUnknownAccounts(limit, after, referenceId);
-}
-| export const UpdateAccountAttributes = object({ | ||
| fields: object({ | ||
| exa_card_tc: boolean(), | ||
| rain_e_sign_consent: boolean(), | ||
| privacy__policy: boolean(), | ||
| account_opening_disclosure: nullable(boolean()), | ||
| economic_activity: string(), | ||
| annual_salary: string(), | ||
| expected_monthly_volume: string(), | ||
| accurate_info_confirmation: boolean(), | ||
| non_unauthorized_solicitation: boolean(), | ||
| non_illegal_activities_2: picklist(["Yes", "No"]), | ||
| address: object({ | ||
| value: object({ | ||
| street_1: string(), | ||
| street_2: nullable(string()), | ||
| city: string(), | ||
| subdivision: nullable(string()), | ||
| postal_code: string(), | ||
| country_code: string(), | ||
| }), | ||
| }), | ||
| }), | ||
| "address-street-1": string(), | ||
| "address-street-2": nullable(string()), | ||
| "address-city": string(), | ||
| "address-subdivision": nullable(string()), | ||
| "address-postal-code": string(), | ||
| "country-code": string(), | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's check the file exists and get its structure
wc -l server/utils/persona.tsRepository: exactly/exa
Length of output: 82
🏁 Script executed:
# Read the specific sections mentioned in the review
sed -n '88,117p' server/utils/persona.tsRepository: exactly/exa
Length of output: 991
🏁 Script executed:
# Read BaseAccountAttributes around line 351
sed -n '340,360p' server/utils/persona.tsRepository: exactly/exa
Length of output: 594
🏁 Script executed:
# Read AccountBasicFields around line 309
sed -n '300,320p' server/utils/persona.tsRepository: exactly/exa
Length of output: 823
🏁 Script executed:
# Search for getAccount function to understand how schemas are used
rg -n "getAccount|\.address.subdivision|address-subdivision" server/utils/persona.ts -A 2 -B 2Repository: exactly/exa
Length of output: 2112
🏁 Script executed:
# Search more broadly for how UpdateAccountAttributes and BaseAccountAttributes are used
rg -n "UpdateAccountAttributes|BaseAccountAttributes" --type=ts -g '!node_modules/**' -A 2 -B 2Repository: exactly/exa
Length of output: 1400
🏁 Script executed:
# Find accountScopeSchemas definition
rg -n "accountScopeSchemas" server/utils/persona.ts -A 10 -B 2Repository: exactly/exa
Length of output: 3137
🏁 Script executed:
# Look for all schema definitions to understand the scope mapping
rg -n "const.*Schemas|accountScopeSchemas\[" server/utils/persona.ts -A 2 -B 1Repository: exactly/exa
Length of output: 1137
🏁 Script executed:
# Search for where UpdateAccountAttributes is actually used
rg -n "updateAccount|UpdateAccountAttributes" --type=ts -g '!node_modules/**' -A 3 -B 1Repository: exactly/exa
Length of output: 2001
subdivision schema mismatch: UpdateAccountAttributes allows nullable but BaseAccountAttributes rejects it.
UpdateAccountAttributes (lines 105, 114) allows subdivision and "address-subdivision" to be null. After updateAccount() writes a null value, calling getAccount(referenceId, "basic") will fail validation because BaseAccountAttributes (line 351) and AccountBasicFields.address.value.subdivision (line 309) both define these fields as non-nullable string().
Update the read schemas to match the write schema:
Proposed fix
address: object({
value: object({
street_1: object({ value: string() }),
street_2: object({ value: nullable(string()) }),
city: object({ value: string() }),
- subdivision: object({ value: string() }),
+ subdivision: object({ value: nullable(string()) }),
postal_code: object({ value: string() }),
country_code: object({ value: string() }),
}),
}),and
- "address-subdivision": string(),
+ "address-subdivision": nullable(string()),| // "address-street-1": string(), | ||
| // "address-street-2": nullable(string()), | ||
| // "address-city": string(), | ||
| // "address-subdivision": string(), | ||
| // "address-postal-code": string(), | ||
| // "address-country-code": string(), |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Remove commented-out code.
These commented-out address attribute lines are dead code. If they're needed later, they can be recovered from version control.
Proposed fix
attributes: object({
status: literal("approved"),
"reference-id": string(),
"redacted-at": nullable(string()),
- // "address-street-1": string(),
- // "address-street-2": nullable(string()),
- // "address-city": string(),
- // "address-subdivision": string(),
- // "address-postal-code": string(),
- // "address-country-code": string(),
fields: pipe(2 participants
Summary by CodeRabbit
Chores
Tests
Changelog