From fe639636b061b1f6d0e216b5eda069f41ea96176 Mon Sep 17 00:00:00 2001 From: Daniel Padrino Date: Fri, 31 Jul 2026 22:01:56 -0300 Subject: [PATCH 1/6] fix(realunit): close the PersonalData KYC step a completed registration satisfies A completed RealUnit registration lifts the account to LEVEL_20, which the KycLevel enum defines as personal data, but never closes the PERSONAL_DATA step that level represents. For an account carrying an open step from an earlier abandoned attempt the step stays IN_PROGRESS forever: createStep's auto-completion is gated on !preventDirectEvaluation, which any prior step row sets, and KycInfoMapper then keeps returning that stale step as currentStep. The RealUnit client cannot render it and dead-ends onboarding. Reconcile the step with the level at every point that concludes the registration is durably in place, including the idempotent retry paths, so an account stuck before this fix repairs itself on its next registration. Scoped to a PENDING step only: preventDirectEvaluation exists so a retry does not paper over a prior rejection, so a FAILED step keeps going through the normal flow. Kept separate from ensureRegistrationKycLevel because that returns early at LEVEL_20 - exactly the state a stuck account is in. --- .../services/__tests__/kyc.service.spec.ts | 91 +++++++++++++++++++ .../generic/kyc/services/kyc.service.ts | 26 ++++++ .../__tests__/realunit.service.spec.ts | 30 ++++++ .../supporting/realunit/realunit.service.ts | 37 +++++++- 4 files changed, 179 insertions(+), 5 deletions(-) diff --git a/src/subdomains/generic/kyc/services/__tests__/kyc.service.spec.ts b/src/subdomains/generic/kyc/services/__tests__/kyc.service.spec.ts index 73d6fdacdc..b0cd7a7949 100644 --- a/src/subdomains/generic/kyc/services/__tests__/kyc.service.spec.ts +++ b/src/subdomains/generic/kyc/services/__tests__/kyc.service.spec.ts @@ -9,6 +9,7 @@ import { Country } from 'src/shared/models/country/country.entity'; import { CountryService } from 'src/shared/models/country/country.service'; import { DfxLogger } from 'src/shared/services/dfx-logger'; import * as processServiceModule from 'src/shared/services/process.service'; +import { AccountType } from '../../../user/models/user-data/account-type.enum'; import { createCustomUserData } from '../../../user/models/user-data/__mocks__/user-data.entity.mock'; import { UserData } from '../../../user/models/user-data/user-data.entity'; import { RiskStatus, UserDataStatus } from '../../../user/models/user-data/user-data.enum'; @@ -795,3 +796,93 @@ describe('KycService checkDfxApproval step promotion', () => { expect(kycStepRepo.update).not.toHaveBeenCalled(); }); }); + +// A flow that writes personal data outside the step machinery (RealUnit registration) leaves the +// PERSONAL_DATA step IN_PROGRESS: createStep's auto-completion is gated on `!preventDirectEvaluation`, +// which any prior step row sets, so an account that once abandoned the step could never satisfy it again +// and KycInfoMapper kept handing that stale step back as `currentStep`. +describe('KycService completeSatisfiedPersonalDataStep', () => { + let service: KycService; + let kycStepRepo: jest.Mocked; + let userDataService: jest.Mocked; + + const personalStep = (status: ReviewStatus): KycStep => + Object.assign(new KycStep(), { id: 72202, name: KycStepName.PERSONAL_DATA, status }); + + // Every field in `requiredKycFields` for a personal account, so `isDataComplete` is true. + const completeUser = (kycSteps: KycStep[], overrides: Partial = {}): UserData => + createCustomUserData({ + id: 315486, + accountType: AccountType.PERSONAL, + mail: 'test@test.com', + phone: '+41790000000', + firstname: 'Erika', + surname: 'Mueller', + street: 'Bahnhofstrasse 1', + location: 'Zurich', + zip: '8001', + kycSteps, + ...overrides, + }); + + beforeEach(() => { + kycStepRepo = createMock(); + userDataService = createMock(); + + service = Object.create(KycService.prototype); + (service as any).kycStepRepo = kycStepRepo; + (service as any).userDataService = userDataService; + jest.spyOn(service as any, 'createStepLog').mockResolvedValue(undefined); + jest.spyOn(service as any, 'updateProgress').mockResolvedValue(undefined); + }); + + const run = async (user: UserData): Promise => { + userDataService.getUserData.mockResolvedValue(user); + // the caller's UserData need not carry `kycSteps`; the method reloads it itself + await service.completeSatisfiedPersonalDataStep(createCustomUserData({ id: user.id })); + }; + + it('completes a pending step and advances the process', async () => { + const step = personalStep(ReviewStatus.IN_PROGRESS); + await run(completeUser([step])); + + expect(userDataService.getUserData).toHaveBeenCalledWith(315486, { kycSteps: true }); + expect(kycStepRepo.update).toHaveBeenCalledTimes(1); + expect(step.status).toBe(ReviewStatus.COMPLETED); + expect(step.getResult()).toMatchObject({ firstname: 'Erika', surname: 'Mueller', zip: '8001' }); + expect((service as any).updateProgress).toHaveBeenCalled(); + }); + + // preventDirectEvaluation exists so a retry does not paper over a prior rejection. A FAILED step must + // keep going through the normal flow rather than being silently resurrected by a registration. + it('leaves a FAILED step untouched', async () => { + const step = personalStep(ReviewStatus.FAILED); + await run(completeUser([step])); + + expect(kycStepRepo.update).not.toHaveBeenCalled(); + expect(step.status).toBe(ReviewStatus.FAILED); + expect((service as any).updateProgress).not.toHaveBeenCalled(); + }); + + it('leaves an already completed step untouched', async () => { + const step = personalStep(ReviewStatus.COMPLETED); + await run(completeUser([step])); + + expect(kycStepRepo.update).not.toHaveBeenCalled(); + }); + + it('does nothing when the account data is incomplete', async () => { + const step = personalStep(ReviewStatus.IN_PROGRESS); + await run(completeUser([step], { surname: undefined })); + + expect(kycStepRepo.update).not.toHaveBeenCalled(); + expect(step.status).toBe(ReviewStatus.IN_PROGRESS); + }); + + it('does nothing when there is no PersonalData step', async () => { + await run(completeUser([])); + + expect(kycStepRepo.update).not.toHaveBeenCalled(); + expect((service as any).updateProgress).not.toHaveBeenCalled(); + }); +}); diff --git a/src/subdomains/generic/kyc/services/kyc.service.ts b/src/subdomains/generic/kyc/services/kyc.service.ts index 5f6cb2dda9..14091096d8 100644 --- a/src/subdomains/generic/kyc/services/kyc.service.ts +++ b/src/subdomains/generic/kyc/services/kyc.service.ts @@ -644,6 +644,32 @@ export class KycService { return KycStepMapper.toStepBase(kycStep); } + /** + * Closes a PERSONAL_DATA step whose data the account already carries, for flows that write personal data + * outside the KYC step machinery (RealUnit registration). Without this the step stays IN_PROGRESS forever: + * the auto-completion in `createStep` is gated on `!preventDirectEvaluation`, which any prior step row sets, + * so an account that once abandoned the step can never satisfy it again — and `KycInfoMapper` keeps handing + * that stale step back as `currentStep`. + * + * Deliberately narrow: only a PENDING (IN_PROGRESS) step is closed, never a FAILED one. `preventDirectEvaluation` + * exists so a retry does not paper over a prior rejection, and that intent is preserved here — a failed step + * stays failed and keeps going through the normal flow. + */ + async completeSatisfiedPersonalDataStep(userData: UserData): Promise { + // The caller's UserData is loaded for its own flow and need not carry `kycSteps`; reload so the step + // lookup never reads an undefined relation (same pattern as checkDfxApproval). + const user = await this.userDataService.getUserData(userData.id, { kycSteps: true }); + + const kycStep = user.getPendingStepWith(KycStepName.PERSONAL_DATA); + if (!kycStep || !user.isDataComplete) return; + + const result = user.requiredKycFields.reduce((prev, curr) => ({ ...prev, [curr]: user[curr] }), {}); + await this.kycStepRepo.update(...kycStep.complete(result)); + await this.createStepLog(user, kycStep); + + await this.updateProgress(user, false); + } + async updateKycStep( kycHash: string, stepId: number, diff --git a/src/subdomains/supporting/realunit/__tests__/realunit.service.spec.ts b/src/subdomains/supporting/realunit/__tests__/realunit.service.spec.ts index 892fa047ce..99263cf993 100644 --- a/src/subdomains/supporting/realunit/__tests__/realunit.service.spec.ts +++ b/src/subdomains/supporting/realunit/__tests__/realunit.service.spec.ts @@ -3771,6 +3771,36 @@ describe('RealUnitService', () => { }); }); + // The stuck account this fixes is BY DEFINITION already at LEVEL_20 (a prior registration lifted it), + // so the step reconciliation must not sit behind ensureRegistrationKycLevel's `>= LEVEL_20` early return. + // Folding the two together would skip the reconciliation for exactly the population that needs it. + describe('ensureRegistrationKycState (reconciles the PersonalData step, not only the level)', () => { + it('closes the PersonalData step even when the account is already at LEVEL_20', async () => { + const completeStep = jest.fn().mockResolvedValue(undefined); + (service as any).kycService.completeSatisfiedPersonalDataStep = completeStep; + const updateUserDataInternal = jest.fn(); + (service as any).userDataService.updateUserDataInternal = updateUserDataInternal; + const userData = { id: 315486, kycLevel: KycLevel.LEVEL_20 } as any; + + await (service as any).ensureRegistrationKycState(userData); + + // the level lift correctly no-ops (already there) ... + expect(updateUserDataInternal).not.toHaveBeenCalled(); + // ... but the step reconciliation still runs + expect(completeStep).toHaveBeenCalledWith(userData); + }); + + it('is best-effort: a step-reconciliation failure never breaks a durable registration', async () => { + (service as any).kycService.completeSatisfiedPersonalDataStep = jest + .fn() + .mockRejectedValue(new Error('step repo down')); + const userData = { id: 315486, kycLevel: KycLevel.LEVEL_20 } as any; + + await expect((service as any).ensureRegistrationKycState(userData)).resolves.toBeUndefined(); + expect((service as any).logger.error).toHaveBeenCalledWith(expect.stringContaining('step repo down')); + }); + }); + describe('describeError (full error body for the PII audit DB log)', () => { it('returns the Aktionariat HTTP error body verbatim when present (the useful, complete part)', () => { const body = { message: 'E-Mail erika.mueller@example.com already registered' }; diff --git a/src/subdomains/supporting/realunit/realunit.service.ts b/src/subdomains/supporting/realunit/realunit.service.ts index 91ec287551..a430fa52f4 100644 --- a/src/subdomains/supporting/realunit/realunit.service.ts +++ b/src/subdomains/supporting/realunit/realunit.service.ts @@ -1358,10 +1358,10 @@ export class RealUnitService { : RealUnitRegistrationStatus.FORWARDING_FAILED; // Self-heal the best-effort KYC level-20 lift on the idempotent COMPLETED retry (see - // ensureRegistrationKycLevel): the prior forward completed the registration, so re-assert the lift + // ensureRegistrationKycState): the prior forward completed the registration, so re-assert the KYC state // here in case it did not land the first time. Monotonic and best-effort — never lowers the level, // never fails the retry. - if (registration.status === ReviewStatus.COMPLETED) await this.ensureRegistrationKycLevel(userData); + if (registration.status === ReviewStatus.COMPLETED) await this.ensureRegistrationKycState(userData); this.logger.info( `RealUnit registration idempotent retry for userData ${userData.id}, registration ${registration.id} → ${status}`, @@ -1644,7 +1644,7 @@ export class RealUnitService { this.logger.info( `RealUnit registration concurrency collision resolved as idempotent for wallet ${dto.walletAddress}`, ); - await this.ensureRegistrationKycLevel(userData); + await this.ensureRegistrationKycState(userData); return true; } // Signature mismatch on an already COMPLETED row must surface as 400, not as a soft forward failure. @@ -1682,8 +1682,8 @@ export class RealUnitService { return false; } - // completed or idempotent: lift KYC level (best-effort, self-healing) and write the INFO audit log. - await this.ensureRegistrationKycLevel(userData); + // completed or idempotent: reconcile KYC state (best-effort, self-healing) and write the INFO audit log. + await this.ensureRegistrationKycState(userData); await this.logAktionariatRegistration( LogSeverity.INFO, dto.walletAddress, @@ -1710,6 +1710,33 @@ export class RealUnitService { } } + // A completed registration means the account's personal data is on file and verified equal to the signed + // envelope (validateRegistrationDto rejects a mismatch with a 400 before we get here), and the level lift + // above grants LEVEL_20 — which the KycLevel enum defines as "personal data". An open PERSONAL_DATA step + // therefore contradicts a decision the API has already made, and the RealUnit client cannot render that step, + // so it dead-ends onboarding. Reconcile the step with the level. + // + // Separate from ensureRegistrationKycLevel on purpose: that one returns early once the account is at + // LEVEL_20, which is exactly the state a stuck account is already in — folding this in would skip it for + // every account that needs it. Best-effort like the lift, and re-asserted on every idempotent retry. + private async ensureRegistrationPersonalDataStep(userData: UserData): Promise { + try { + await this.kycService.completeSatisfiedPersonalDataStep(userData); + } catch (e) { + this.logger.error( + `Failed to close the PersonalData KYC step for RealUnit registration (userData ${userData.id}); will self-heal on retry: ${e?.message || e}`, + ); + } + } + + // The KYC state a durably COMPLETED registration implies. Called from every point that concludes the + // registration is in place, including the idempotent retry paths — which is what lets an account stuck + // before this fix repair itself the next time it registers. + private async ensureRegistrationKycState(userData: UserData): Promise { + await this.ensureRegistrationKycLevel(userData); + await this.ensureRegistrationPersonalDataStep(userData); + } + // Aktionariat's registerUser answers "Existing user found, updated your address." when the signed email // already belongs to a share-register shareholder: it updates that shareholder's wallet in place and sends // NO confirmation email (a newly registered email instead gets "Confirmation email sent to ..."). Such a From 6db35055a28560b34684f05f62aaa12d0fa2c256 Mon Sep 17 00:00:00 2001 From: Daniel Padrino Date: Fri, 31 Jul 2026 22:25:02 -0300 Subject: [PATCH 2/6] fix(kyc): do not auto-close a PersonalData step re-opened by a rejection Review follow-ups on the registration reconciliation. isDataComplete is a non-null check, not a validity check, so the pending lookup alone could not tell an abandoned step from one restartStep deliberately re-opened after Sumsub reported PROBLEMATIC_APPLICANT_DATA. The rejection marker lives on the failed row, not the pending one, so the retry was matched and would have been re-completed with the same rejected data - skipping the correction step and stranding the user on Ident. Bail whenever the PersonalData chain carries a failed step. The RealUnit spec provided KycService as a bare object, so the new call was an undefined-function TypeError swallowed by the best-effort catch: the three call sites were unpinned and an existing lift-failure assertion had stopped discriminating. Provide the method, pin the call sites, and tighten that assertion to the lift-specific message. Move the requiredKycFields projection onto UserData as kycFieldData and use it at both call sites, correct the stale createStep reference to initiateStep, drop production identifiers from the fixtures, and remove the self-heal claim: an already-wedged account answers AlreadyRegistered and never re-posts register/complete, so this prevents the wedge rather than curing it. --- .../services/__tests__/kyc.service.spec.ts | 22 +++++++++--- .../generic/kyc/services/kyc.service.ts | 23 +++++++----- .../user/models/user-data/user-data.entity.ts | 5 +++ .../__tests__/realunit.service.spec.ts | 35 ++++++++++++++++--- .../supporting/realunit/realunit.service.ts | 9 +++-- 5 files changed, 75 insertions(+), 19 deletions(-) diff --git a/src/subdomains/generic/kyc/services/__tests__/kyc.service.spec.ts b/src/subdomains/generic/kyc/services/__tests__/kyc.service.spec.ts index b0cd7a7949..44fb89034a 100644 --- a/src/subdomains/generic/kyc/services/__tests__/kyc.service.spec.ts +++ b/src/subdomains/generic/kyc/services/__tests__/kyc.service.spec.ts @@ -798,7 +798,7 @@ describe('KycService checkDfxApproval step promotion', () => { }); // A flow that writes personal data outside the step machinery (RealUnit registration) leaves the -// PERSONAL_DATA step IN_PROGRESS: createStep's auto-completion is gated on `!preventDirectEvaluation`, +// PERSONAL_DATA step IN_PROGRESS: initiateStep's auto-completion is gated on `!preventDirectEvaluation`, // which any prior step row sets, so an account that once abandoned the step could never satisfy it again // and KycInfoMapper kept handing that stale step back as `currentStep`. describe('KycService completeSatisfiedPersonalDataStep', () => { @@ -807,12 +807,12 @@ describe('KycService completeSatisfiedPersonalDataStep', () => { let userDataService: jest.Mocked; const personalStep = (status: ReviewStatus): KycStep => - Object.assign(new KycStep(), { id: 72202, name: KycStepName.PERSONAL_DATA, status }); + Object.assign(new KycStep(), { id: 2, name: KycStepName.PERSONAL_DATA, status }); // Every field in `requiredKycFields` for a personal account, so `isDataComplete` is true. const completeUser = (kycSteps: KycStep[], overrides: Partial = {}): UserData => createCustomUserData({ - id: 315486, + id: 1, accountType: AccountType.PERSONAL, mail: 'test@test.com', phone: '+41790000000', @@ -846,7 +846,7 @@ describe('KycService completeSatisfiedPersonalDataStep', () => { const step = personalStep(ReviewStatus.IN_PROGRESS); await run(completeUser([step])); - expect(userDataService.getUserData).toHaveBeenCalledWith(315486, { kycSteps: true }); + expect(userDataService.getUserData).toHaveBeenCalledWith(1, { kycSteps: true }); expect(kycStepRepo.update).toHaveBeenCalledTimes(1); expect(step.status).toBe(ReviewStatus.COMPLETED); expect(step.getResult()).toMatchObject({ firstname: 'Erika', surname: 'Mueller', zip: '8001' }); @@ -864,6 +864,20 @@ describe('KycService completeSatisfiedPersonalDataStep', () => { expect((service as any).updateProgress).not.toHaveBeenCalled(); }); + // When Sumsub reports PROBLEMATIC_APPLICANT_DATA, restartStep FAILS the completed step and opens a fresh + // IN_PROGRESS one so the user can correct data that is present but wrong. isDataComplete is a non-null + // check and cannot see that, so without the failed-step guard the retry would be auto-completed with the + // same rejected data and the user stranded on IDENT instead of the correction step. + it('leaves a step re-opened by a rejection alone (FAILED + fresh IN_PROGRESS chain)', async () => { + const failed = personalStep(ReviewStatus.FAILED); + const reopened = personalStep(ReviewStatus.IN_PROGRESS); + await run(completeUser([failed, reopened])); + + expect(kycStepRepo.update).not.toHaveBeenCalled(); + expect(reopened.status).toBe(ReviewStatus.IN_PROGRESS); + expect((service as any).updateProgress).not.toHaveBeenCalled(); + }); + it('leaves an already completed step untouched', async () => { const step = personalStep(ReviewStatus.COMPLETED); await run(completeUser([step])); diff --git a/src/subdomains/generic/kyc/services/kyc.service.ts b/src/subdomains/generic/kyc/services/kyc.service.ts index 14091096d8..9d5663c7f4 100644 --- a/src/subdomains/generic/kyc/services/kyc.service.ts +++ b/src/subdomains/generic/kyc/services/kyc.service.ts @@ -647,24 +647,30 @@ export class KycService { /** * Closes a PERSONAL_DATA step whose data the account already carries, for flows that write personal data * outside the KYC step machinery (RealUnit registration). Without this the step stays IN_PROGRESS forever: - * the auto-completion in `createStep` is gated on `!preventDirectEvaluation`, which any prior step row sets, + * the auto-completion in `initiateStep` is gated on `!preventDirectEvaluation`, which any prior step row sets, * so an account that once abandoned the step can never satisfy it again — and `KycInfoMapper` keeps handing * that stale step back as `currentStep`. * - * Deliberately narrow: only a PENDING (IN_PROGRESS) step is closed, never a FAILED one. `preventDirectEvaluation` - * exists so a retry does not paper over a prior rejection, and that intent is preserved here — a failed step - * stays failed and keeps going through the normal flow. + * Only ever closes a step that was abandoned, never one that was RE-OPENED. `isDataComplete` is a non-null + * check, not a validity check, so it cannot tell the two apart on its own: when Sumsub reports + * PROBLEMATIC_APPLICANT_DATA, `restartStep` FAILS the completed step and opens a fresh IN_PROGRESS one so the + * user can correct data that is present but wrong. Auto-completing that retry with the same unchanged data + * would skip the correction step and strand the user on IDENT instead. The rejection marker lives on the + * failed row, not on the pending one, so the pending lookup alone is no protection — any FAILED step in the + * chain means this account was deliberately sent back through the normal flow, and we leave it there. */ async completeSatisfiedPersonalDataStep(userData: UserData): Promise { // The caller's UserData is loaded for its own flow and need not carry `kycSteps`; reload so the step // lookup never reads an undefined relation (same pattern as checkDfxApproval). const user = await this.userDataService.getUserData(userData.id, { kycSteps: true }); - const kycStep = user.getPendingStepWith(KycStepName.PERSONAL_DATA); + const steps = user.getStepsWith(KycStepName.PERSONAL_DATA); + if (steps.some((s) => s.isFailed)) return; + + const kycStep = steps.find((s) => s.isInProgress); if (!kycStep || !user.isDataComplete) return; - const result = user.requiredKycFields.reduce((prev, curr) => ({ ...prev, [curr]: user[curr] }), {}); - await this.kycStepRepo.update(...kycStep.complete(result)); + await this.kycStepRepo.update(...kycStep.complete(user.kycFieldData)); await this.createStepLog(user, kycStep); await this.updateProgress(user, false); @@ -1384,8 +1390,7 @@ export class KycService { const completedStep = user.getStepsWith(KycStepName.PERSONAL_DATA).find((s) => s.isCompleted); if (completedStep) await this.kycStepRepo.update(...completedStep.cancel()); - const result = user.requiredKycFields.reduce((prev, curr) => ({ ...prev, [curr]: user[curr] }), {}); - if (user.isDataComplete && !preventDirectEvaluation) kycStep.complete(result); + if (user.isDataComplete && !preventDirectEvaluation) kycStep.complete(user.kycFieldData); break; } diff --git a/src/subdomains/generic/user/models/user-data/user-data.entity.ts b/src/subdomains/generic/user/models/user-data/user-data.entity.ts index 822cfc2740..527ff66a6b 100644 --- a/src/subdomains/generic/user/models/user-data/user-data.entity.ts +++ b/src/subdomains/generic/user/models/user-data/user-data.entity.ts @@ -868,6 +868,11 @@ export class UserData extends IEntity { return this.requiredKycFields.every((f) => this[f]); } + // The requiredKycFields projection a completed PERSONAL_DATA step stores as its result. + get kycFieldData(): Record { + return this.requiredKycFields.reduce((prev, curr) => ({ ...prev, [curr]: this[curr] }), {}); + } + get requiredInvoiceFields(): string[] { return ['accountType'].concat(this.isPersonalAccount ? ['firstname', 'surname'] : ['organizationName']); } diff --git a/src/subdomains/supporting/realunit/__tests__/realunit.service.spec.ts b/src/subdomains/supporting/realunit/__tests__/realunit.service.spec.ts index 99263cf993..0fa6bac304 100644 --- a/src/subdomains/supporting/realunit/__tests__/realunit.service.spec.ts +++ b/src/subdomains/supporting/realunit/__tests__/realunit.service.spec.ts @@ -288,7 +288,7 @@ describe('RealUnitService', () => { getUserByAddress: jest.fn(), }, }, - { provide: KycService, useValue: {} }, + { provide: KycService, useValue: { completeSatisfiedPersonalDataStep: jest.fn() } }, { provide: CountryService, useValue: { getCountryWithSymbol: jest.fn() } }, { provide: LanguageService, useValue: { getLanguageBySymbol: jest.fn() } }, { provide: HttpService, useValue: { post: jest.fn(), getRaw: jest.fn() } }, @@ -3054,6 +3054,21 @@ describe('RealUnitService', () => { // REGRESSION GUARD: a legacy software wallet that signed the raw UTF-8 fields // (still accepted by verifyRealUnitRegistrationSignature) must keep working — // the forward must stay UTF-8, not be transliterated, or Aktionariat rejects it. + // Pins the forwardRegistration call site: the pre-existing spies target ensureRegistrationKycLevel, + // which ensureRegistrationKycState still calls transitively, so reverting the call site would otherwise + // keep the suite green and silently undo the fix. + it('reconciles the PersonalData step after a completed forward', async () => { + const wallet = softwareWallet.address; + const signature = await softwareWallet._signTypedData(domain, types, utf8Fields(wallet)); + const dto = buildDto(utf8Fields(wallet), signature); + httpService.post.mockResolvedValue({} as any); + + const ok = await (service as any).forwardRegistration(fakeUserData(), dto); + + expect(ok).toBe(true); + expect((service as any).kycService.completeSatisfiedPersonalDataStep).toHaveBeenCalled(); + }); + it('forwards the raw UTF-8 fields unchanged when the wallet signed UTF-8 (legacy app)', async () => { const wallet = softwareWallet.address; const signature = await softwareWallet._signTypedData(domain, types, utf8Fields(wallet)); @@ -3730,7 +3745,7 @@ describe('RealUnitService', () => { // the COMPLETED persist already committed, so a failed lift must not fail the registration expect(ok).toBe(true); expect(aktionariatTxManager.save).toHaveBeenCalledTimes(1); - expect((service as any).logger.error).toHaveBeenCalledWith(expect.stringContaining('will self-heal on retry')); + expect((service as any).logger.error).toHaveBeenCalledWith(expect.stringContaining('Failed to lift KYC level')); }); it('keeps the registration when the KYC lift rejects without a message', async () => { @@ -3780,7 +3795,7 @@ describe('RealUnitService', () => { (service as any).kycService.completeSatisfiedPersonalDataStep = completeStep; const updateUserDataInternal = jest.fn(); (service as any).userDataService.updateUserDataInternal = updateUserDataInternal; - const userData = { id: 315486, kycLevel: KycLevel.LEVEL_20 } as any; + const userData = { id: 1, kycLevel: KycLevel.LEVEL_20 } as any; await (service as any).ensureRegistrationKycState(userData); @@ -3790,11 +3805,23 @@ describe('RealUnitService', () => { expect(completeStep).toHaveBeenCalledWith(userData); }); + // Pins the idempotent call site. The pre-existing spies target ensureRegistrationKycLevel, which + // ensureRegistrationKycState still calls transitively, so reverting the call sites would otherwise keep + // the suite green and silently undo the fix. The forwardRegistration site is pinned in its own describe. + it('is reached from idempotentRegistrationResult on a COMPLETED registration', async () => { + const userData = { id: 1, kycLevel: KycLevel.LEVEL_20 } as any; + const registration = { id: 2, signature: '0xsig', status: ReviewStatus.COMPLETED } as any; + + await (service as any).idempotentRegistrationResult(userData, registration, '0xsig'); + + expect((service as any).kycService.completeSatisfiedPersonalDataStep).toHaveBeenCalledWith(userData); + }); + it('is best-effort: a step-reconciliation failure never breaks a durable registration', async () => { (service as any).kycService.completeSatisfiedPersonalDataStep = jest .fn() .mockRejectedValue(new Error('step repo down')); - const userData = { id: 315486, kycLevel: KycLevel.LEVEL_20 } as any; + const userData = { id: 1, kycLevel: KycLevel.LEVEL_20 } as any; await expect((service as any).ensureRegistrationKycState(userData)).resolves.toBeUndefined(); expect((service as any).logger.error).toHaveBeenCalledWith(expect.stringContaining('step repo down')); diff --git a/src/subdomains/supporting/realunit/realunit.service.ts b/src/subdomains/supporting/realunit/realunit.service.ts index a430fa52f4..8c899011fb 100644 --- a/src/subdomains/supporting/realunit/realunit.service.ts +++ b/src/subdomains/supporting/realunit/realunit.service.ts @@ -1730,8 +1730,13 @@ export class RealUnitService { } // The KYC state a durably COMPLETED registration implies. Called from every point that concludes the - // registration is in place, including the idempotent retry paths — which is what lets an account stuck - // before this fix repair itself the next time it registers. + // registration is in place, including the idempotent retry paths. + // + // Scope: this reconciles registrations as they happen, so it prevents the wedge rather than curing it. An + // account that is ALREADY wedged does not come back through here on its own — for a wallet with a COMPLETED + // registration `getRegistrationInfo` answers AlreadyRegistered, and the client then goes straight to the KYC + // step flow without re-posting register/complete. Such an account is only reached if it registers a further + // wallet. The pre-existing backlog is handled out of band. private async ensureRegistrationKycState(userData: UserData): Promise { await this.ensureRegistrationKycLevel(userData); await this.ensureRegistrationPersonalDataStep(userData); From 7357345a6f30435f5a9a68fe7ff0dfa4c021db28 Mon Sep 17 00:00:00 2001 From: Daniel Padrino Date: Fri, 31 Jul 2026 22:48:09 -0300 Subject: [PATCH 3/6] fix(kyc): judge the PersonalData chain by its most recent settled step Second review pass. The failed-chain guard judged the whole history, so a rejection the user had already remedied would disable the reconciliation for that account forever. Take the verdict from the most recent settled step instead: an unremedied re-open is still blocked, a remedied one stays eligible. Pin the concurrency-collision call site. The existing spy targets ensureRegistrationKycLevel, which ensureRegistrationKycState calls transitively, so reverting that site kept the suite green - the same trap the other two pinning tests were written to close. All three sites now fail a test individually when reverted. Drop the remaining self-heal wording from the log message and the two comments this PR added. It is contradicted by the scope note eight lines below: when the reconciliation throws, the account is left wedged and the log would have told the on-call it recovers by itself, suppressing the manual follow-up that is actually needed. Re-pair the new forwardRegistration test with its own comment so the pre-existing REGRESSION GUARD rationale sits with the test it documents. --- .../services/__tests__/kyc.service.spec.ts | 21 +++++++++++++++---- .../generic/kyc/services/kyc.service.ts | 14 ++++++++++--- .../__tests__/realunit.service.spec.ts | 13 ++++++++---- .../supporting/realunit/realunit.service.ts | 6 +++--- 4 files changed, 40 insertions(+), 14 deletions(-) diff --git a/src/subdomains/generic/kyc/services/__tests__/kyc.service.spec.ts b/src/subdomains/generic/kyc/services/__tests__/kyc.service.spec.ts index 44fb89034a..562e6ba9ae 100644 --- a/src/subdomains/generic/kyc/services/__tests__/kyc.service.spec.ts +++ b/src/subdomains/generic/kyc/services/__tests__/kyc.service.spec.ts @@ -806,8 +806,8 @@ describe('KycService completeSatisfiedPersonalDataStep', () => { let kycStepRepo: jest.Mocked; let userDataService: jest.Mocked; - const personalStep = (status: ReviewStatus): KycStep => - Object.assign(new KycStep(), { id: 2, name: KycStepName.PERSONAL_DATA, status }); + const personalStep = (status: ReviewStatus, sequenceNumber = 0): KycStep => + Object.assign(new KycStep(), { id: 2 + sequenceNumber, name: KycStepName.PERSONAL_DATA, status, sequenceNumber }); // Every field in `requiredKycFields` for a personal account, so `isDataComplete` is true. const completeUser = (kycSteps: KycStep[], overrides: Partial = {}): UserData => @@ -869,8 +869,8 @@ describe('KycService completeSatisfiedPersonalDataStep', () => { // check and cannot see that, so without the failed-step guard the retry would be auto-completed with the // same rejected data and the user stranded on IDENT instead of the correction step. it('leaves a step re-opened by a rejection alone (FAILED + fresh IN_PROGRESS chain)', async () => { - const failed = personalStep(ReviewStatus.FAILED); - const reopened = personalStep(ReviewStatus.IN_PROGRESS); + const failed = personalStep(ReviewStatus.FAILED, 0); + const reopened = personalStep(ReviewStatus.IN_PROGRESS, 1); await run(completeUser([failed, reopened])); expect(kycStepRepo.update).not.toHaveBeenCalled(); @@ -878,6 +878,19 @@ describe('KycService completeSatisfiedPersonalDataStep', () => { expect((service as any).updateProgress).not.toHaveBeenCalled(); }); + // A rejection the user has since remedied ends in a completed (then cancelled) step. Judging the whole + // history would disable the reconciliation for that account forever, so the verdict comes from the most + // recent SETTLED step only — here a cancelled one, so the account stays eligible. + it('still completes when an older rejection was remedied before the step was re-opened', async () => { + const failed = personalStep(ReviewStatus.FAILED, 0); + const remedied = personalStep(ReviewStatus.CANCELED, 1); + const pending = personalStep(ReviewStatus.IN_PROGRESS, 2); + await run(completeUser([failed, remedied, pending])); + + expect(kycStepRepo.update).toHaveBeenCalledTimes(1); + expect(pending.status).toBe(ReviewStatus.COMPLETED); + }); + it('leaves an already completed step untouched', async () => { const step = personalStep(ReviewStatus.COMPLETED); await run(completeUser([step])); diff --git a/src/subdomains/generic/kyc/services/kyc.service.ts b/src/subdomains/generic/kyc/services/kyc.service.ts index 9d5663c7f4..168eb547f9 100644 --- a/src/subdomains/generic/kyc/services/kyc.service.ts +++ b/src/subdomains/generic/kyc/services/kyc.service.ts @@ -656,8 +656,12 @@ export class KycService { * PROBLEMATIC_APPLICANT_DATA, `restartStep` FAILS the completed step and opens a fresh IN_PROGRESS one so the * user can correct data that is present but wrong. Auto-completing that retry with the same unchanged data * would skip the correction step and strand the user on IDENT instead. The rejection marker lives on the - * failed row, not on the pending one, so the pending lookup alone is no protection — any FAILED step in the - * chain means this account was deliberately sent back through the normal flow, and we leave it there. + * failed row, not on the pending one, so the pending lookup alone is no protection. + * + * The verdict comes from the most recent SETTLED step, not from the whole history: a rejection the user has + * since remedied ends in a completed (then cancelled) step, and that account must stay eligible. Only a + * chain whose latest settled step is still FAILED is an unremedied re-open, and it keeps going through the + * normal flow. */ async completeSatisfiedPersonalDataStep(userData: UserData): Promise { // The caller's UserData is loaded for its own flow and need not carry `kycSteps`; reload so the step @@ -665,7 +669,11 @@ export class KycService { const user = await this.userDataService.getUserData(userData.id, { kycSteps: true }); const steps = user.getStepsWith(KycStepName.PERSONAL_DATA); - if (steps.some((s) => s.isFailed)) return; + const lastSettled = Util.maxObj( + steps.filter((s) => !s.isInProgress), + 'sequenceNumber', + ); + if (lastSettled?.isFailed) return; const kycStep = steps.find((s) => s.isInProgress); if (!kycStep || !user.isDataComplete) return; diff --git a/src/subdomains/supporting/realunit/__tests__/realunit.service.spec.ts b/src/subdomains/supporting/realunit/__tests__/realunit.service.spec.ts index 0fa6bac304..b77151ed7c 100644 --- a/src/subdomains/supporting/realunit/__tests__/realunit.service.spec.ts +++ b/src/subdomains/supporting/realunit/__tests__/realunit.service.spec.ts @@ -3051,9 +3051,6 @@ describe('RealUnitService', () => { mockEnvironment = 'loc'; }); - // REGRESSION GUARD: a legacy software wallet that signed the raw UTF-8 fields - // (still accepted by verifyRealUnitRegistrationSignature) must keep working — - // the forward must stay UTF-8, not be transliterated, or Aktionariat rejects it. // Pins the forwardRegistration call site: the pre-existing spies target ensureRegistrationKycLevel, // which ensureRegistrationKycState still calls transitively, so reverting the call site would otherwise // keep the suite green and silently undo the fix. @@ -3069,6 +3066,9 @@ describe('RealUnitService', () => { expect((service as any).kycService.completeSatisfiedPersonalDataStep).toHaveBeenCalled(); }); + // REGRESSION GUARD: a legacy software wallet that signed the raw UTF-8 fields + // (still accepted by verifyRealUnitRegistrationSignature) must keep working — + // the forward must stay UTF-8, not be transliterated, or Aktionariat rejects it. it('forwards the raw UTF-8 fields unchanged when the wallet signed UTF-8 (legacy app)', async () => { const wallet = softwareWallet.address; const signature = await softwareWallet._signTypedData(domain, types, utf8Fields(wallet)); @@ -3551,8 +3551,13 @@ describe('RealUnitService', () => { expect(ok).toBe(true); // the collision is NOT recorded as a failure expect(logService.create).not.toHaveBeenCalledWith(expect.objectContaining({ severity: LogSeverity.ERROR })); - // the idempotent-collision path still (best-effort) lifts the KYC level + // the idempotent-collision path still (best-effort) reconciles the KYC state. Both halves are asserted: + // the level spy alone passes even if the call site is reverted, because ensureRegistrationKycState calls + // ensureRegistrationKycLevel transitively. expect(ensureSpy).toHaveBeenCalledWith(expect.objectContaining({ id: 1 })); + expect((service as any).kycService.completeSatisfiedPersonalDataStep).toHaveBeenCalledWith( + expect.objectContaining({ id: 1 }), + ); }); it('writes the full Aktionariat error body to the DB log but keeps the Loki line redacted', async () => { diff --git a/src/subdomains/supporting/realunit/realunit.service.ts b/src/subdomains/supporting/realunit/realunit.service.ts index 8c899011fb..a87618df57 100644 --- a/src/subdomains/supporting/realunit/realunit.service.ts +++ b/src/subdomains/supporting/realunit/realunit.service.ts @@ -1682,7 +1682,7 @@ export class RealUnitService { return false; } - // completed or idempotent: reconcile KYC state (best-effort, self-healing) and write the INFO audit log. + // completed or idempotent: reconcile KYC state (best-effort) and write the INFO audit log. await this.ensureRegistrationKycState(userData); await this.logAktionariatRegistration( LogSeverity.INFO, @@ -1718,13 +1718,13 @@ export class RealUnitService { // // Separate from ensureRegistrationKycLevel on purpose: that one returns early once the account is at // LEVEL_20, which is exactly the state a stuck account is already in — folding this in would skip it for - // every account that needs it. Best-effort like the lift, and re-asserted on every idempotent retry. + // every account that needs it. Best-effort like the lift, so a failure here never fails the registration. private async ensureRegistrationPersonalDataStep(userData: UserData): Promise { try { await this.kycService.completeSatisfiedPersonalDataStep(userData); } catch (e) { this.logger.error( - `Failed to close the PersonalData KYC step for RealUnit registration (userData ${userData.id}); will self-heal on retry: ${e?.message || e}`, + `Failed to close the PersonalData KYC step for RealUnit registration (userData ${userData.id}); the step stays open and needs manual reconciliation: ${e?.message || e}`, ); } } From 6f98f9abdb8c3d53983283e8d1bc68735de3fb6b Mon Sep 17 00:00:00 2001 From: Daniel Padrino Date: Fri, 31 Jul 2026 23:04:20 -0300 Subject: [PATCH 4/6] fix(kyc): discriminate a remedied rejection by step result, not by status Third review pass. Both reviews independently found that the previous pass reopened the regression the guard exists to prevent. CANCELED cannot stand for remediation: initiateStep cancels the previous COMPLETED step, but it also cancels a merely PENDING one, so an untouched rejection retry and a genuine remediation end in the same status. Taking CANCELED as proof let a Sumsub-rejected retry be auto-completed with the same data - and the test added last pass asserted that wrong outcome, because its fixture was the unremedied shape. result separates them durably: complete() writes it, cancel() leaves it alone, and a step cancelled while still pending never had one. A result-less cancellation is skipped so the failed step behind it still decides. The fixture now carries a result and gains its negative twin. Pick the pending step by highest sequence rather than by first match: legacy merged-in accounts carry a second IN_PROGRESS step at a negative sequence, and find() would close that dead step while leaving the live one open - reporting success on an account that stayed wedged. --- .../services/__tests__/kyc.service.spec.ts | 40 ++++++++++++++++--- .../generic/kyc/services/kyc.service.ts | 20 +++++++--- 2 files changed, 49 insertions(+), 11 deletions(-) diff --git a/src/subdomains/generic/kyc/services/__tests__/kyc.service.spec.ts b/src/subdomains/generic/kyc/services/__tests__/kyc.service.spec.ts index 562e6ba9ae..d4767f50de 100644 --- a/src/subdomains/generic/kyc/services/__tests__/kyc.service.spec.ts +++ b/src/subdomains/generic/kyc/services/__tests__/kyc.service.spec.ts @@ -806,8 +806,14 @@ describe('KycService completeSatisfiedPersonalDataStep', () => { let kycStepRepo: jest.Mocked; let userDataService: jest.Mocked; - const personalStep = (status: ReviewStatus, sequenceNumber = 0): KycStep => - Object.assign(new KycStep(), { id: 2 + sequenceNumber, name: KycStepName.PERSONAL_DATA, status, sequenceNumber }); + const personalStep = (status: ReviewStatus, sequenceNumber = 0, result?: string): KycStep => + Object.assign(new KycStep(), { + id: 2 + sequenceNumber, + name: KycStepName.PERSONAL_DATA, + status, + sequenceNumber, + result, + }); // Every field in `requiredKycFields` for a personal account, so `isDataComplete` is true. const completeUser = (kycSteps: KycStep[], overrides: Partial = {}): UserData => @@ -878,12 +884,11 @@ describe('KycService completeSatisfiedPersonalDataStep', () => { expect((service as any).updateProgress).not.toHaveBeenCalled(); }); - // A rejection the user has since remedied ends in a completed (then cancelled) step. Judging the whole - // history would disable the reconciliation for that account forever, so the verdict comes from the most - // recent SETTLED step only — here a cancelled one, so the account stays eligible. + // A rejection the user has since remedied ends in a step that COMPLETED and was later cancelled by + // initiateStep. cancel() leaves `result` in place, so that row still carries the proof it was satisfied. it('still completes when an older rejection was remedied before the step was re-opened', async () => { const failed = personalStep(ReviewStatus.FAILED, 0); - const remedied = personalStep(ReviewStatus.CANCELED, 1); + const remedied = personalStep(ReviewStatus.CANCELED, 1, '{"firstname":"Erika"}'); const pending = personalStep(ReviewStatus.IN_PROGRESS, 2); await run(completeUser([failed, remedied, pending])); @@ -891,6 +896,29 @@ describe('KycService completeSatisfiedPersonalDataStep', () => { expect(pending.status).toBe(ReviewStatus.COMPLETED); }); + // The negative twin. initiateStep also cancels a merely PENDING step, so a CANCELED row with no result is + // an untouched retry, not a remediation — the FAILED step behind it must still block. + it('leaves the chain alone when the cancelled step never completed (no result)', async () => { + const failed = personalStep(ReviewStatus.FAILED, 0); + const untouched = personalStep(ReviewStatus.CANCELED, 1); + const pending = personalStep(ReviewStatus.IN_PROGRESS, 2); + await run(completeUser([failed, untouched, pending])); + + expect(kycStepRepo.update).not.toHaveBeenCalled(); + expect(pending.status).toBe(ReviewStatus.IN_PROGRESS); + }); + + // Legacy merged-in accounts can carry two IN_PROGRESS steps, the merged-in one at a negative sequence. + // Closing that dead step would leave the live one open and the account still wedged. + it('closes the highest-sequence pending step when a merged-in one is also open', async () => { + const merged = personalStep(ReviewStatus.IN_PROGRESS, -102); + const live = personalStep(ReviewStatus.IN_PROGRESS, 0); + await run(completeUser([merged, live])); + + expect(live.status).toBe(ReviewStatus.COMPLETED); + expect(merged.status).toBe(ReviewStatus.IN_PROGRESS); + }); + it('leaves an already completed step untouched', async () => { const step = personalStep(ReviewStatus.COMPLETED); await run(completeUser([step])); diff --git a/src/subdomains/generic/kyc/services/kyc.service.ts b/src/subdomains/generic/kyc/services/kyc.service.ts index 168eb547f9..ec65c20585 100644 --- a/src/subdomains/generic/kyc/services/kyc.service.ts +++ b/src/subdomains/generic/kyc/services/kyc.service.ts @@ -659,9 +659,13 @@ export class KycService { * failed row, not on the pending one, so the pending lookup alone is no protection. * * The verdict comes from the most recent SETTLED step, not from the whole history: a rejection the user has - * since remedied ends in a completed (then cancelled) step, and that account must stay eligible. Only a - * chain whose latest settled step is still FAILED is an unremedied re-open, and it keeps going through the - * normal flow. + * since remedied ends in a completed step, and that account must stay eligible. + * + * CANCELED alone cannot stand for "remedied": `initiateStep` cancels the previous COMPLETED step (:1359) but + * also cancels a merely PENDING one (:1349), so both a remediation and an untouched retry end up CANCELED. + * `result` separates them durably — `complete()` writes it and `cancel()` leaves it alone, while a step + * cancelled while still pending never had one. A result-less cancellation is therefore not evidence of + * anything and is skipped, so the FAILED step behind it still decides. */ async completeSatisfiedPersonalDataStep(userData: UserData): Promise { // The caller's UserData is loaded for its own flow and need not carry `kycSteps`; reload so the step @@ -670,12 +674,18 @@ export class KycService { const steps = user.getStepsWith(KycStepName.PERSONAL_DATA); const lastSettled = Util.maxObj( - steps.filter((s) => !s.isInProgress), + steps.filter((s) => !s.isInProgress && !(s.isCanceled && !s.result)), 'sequenceNumber', ); if (lastSettled?.isFailed) return; - const kycStep = steps.find((s) => s.isInProgress); + // Highest sequence, not the first match: legacy merged-in accounts can carry more than one IN_PROGRESS + // step, and the merged-in ones sit at negative sequence numbers — `find` would close a dead step and + // leave the live one open. + const kycStep = Util.maxObj( + steps.filter((s) => s.isInProgress), + 'sequenceNumber', + ); if (!kycStep || !user.isDataComplete) return; await this.kycStepRepo.update(...kycStep.complete(user.kycFieldData)); From 0d94b988d9c7bb03870165056a1c47ba27eadad1 Mon Sep 17 00:00:00 2001 From: Daniel Padrino Date: Fri, 31 Jul 2026 23:17:16 -0300 Subject: [PATCH 5/6] fix(kyc): decide the PersonalData verdict from the account's own chain Fourth review pass. Merged-in rows were still in scope for both the verdict and the pick. A merge seeds the slave's steps 100 below the floor and orders them chronologically only within a batch, so a later merge sits below an earlier one: ranking across batches could let a stale cancellation outrank a newer rejection, and an account whose only pending step was merged-in would have had that dead row closed while its live chain stayed untouched. Own rows always start at 0 and merged-in rows are always negative, so the split is exact. Move the settled-verdict predicate onto KycStep as hasSettledVerdict, where the rest of the status getters live, and drop the hardcoded line references from the docstring - this PR's own insertions had already shifted them onto unrelated code. --- .../generic/kyc/entities/kyc-step.entity.ts | 7 +++++++ .../services/__tests__/kyc.service.spec.ts | 13 +++++++++++- .../generic/kyc/services/kyc.service.ts | 20 +++++++++---------- 3 files changed, 29 insertions(+), 11 deletions(-) diff --git a/src/subdomains/generic/kyc/entities/kyc-step.entity.ts b/src/subdomains/generic/kyc/entities/kyc-step.entity.ts index 8f96031946..1fd3101f46 100644 --- a/src/subdomains/generic/kyc/entities/kyc-step.entity.ts +++ b/src/subdomains/generic/kyc/entities/kyc-step.entity.ts @@ -213,6 +213,13 @@ export class KycStep extends IEntity { return this.isInReview || this.isCompleted; } + // Whether this row records an outcome. A cancellation only does so if the step had completed first: + // `complete()` writes `result` and `cancel()` leaves it untouched, so a cancelled step without one was + // never satisfied — it was merely superseded, and says nothing about the attempt it replaced. + get hasSettledVerdict(): boolean { + return !this.isInProgress && !(this.isCanceled && !this.result); + } + update( status: ReviewStatus, result?: KycStepResult, diff --git a/src/subdomains/generic/kyc/services/__tests__/kyc.service.spec.ts b/src/subdomains/generic/kyc/services/__tests__/kyc.service.spec.ts index d4767f50de..3d5f10bf41 100644 --- a/src/subdomains/generic/kyc/services/__tests__/kyc.service.spec.ts +++ b/src/subdomains/generic/kyc/services/__tests__/kyc.service.spec.ts @@ -910,7 +910,7 @@ describe('KycService completeSatisfiedPersonalDataStep', () => { // Legacy merged-in accounts can carry two IN_PROGRESS steps, the merged-in one at a negative sequence. // Closing that dead step would leave the live one open and the account still wedged. - it('closes the highest-sequence pending step when a merged-in one is also open', async () => { + it('closes the live pending step and ignores a merged-in one', async () => { const merged = personalStep(ReviewStatus.IN_PROGRESS, -102); const live = personalStep(ReviewStatus.IN_PROGRESS, 0); await run(completeUser([merged, live])); @@ -919,6 +919,17 @@ describe('KycService completeSatisfiedPersonalDataStep', () => { expect(merged.status).toBe(ReviewStatus.IN_PROGRESS); }); + // Merged-in rows are history, not the account's own chain: a merge seeds them 100 below the floor, and + // batches are ordered chronologically only WITHIN a batch. Left in scope they would both vote on the + // verdict and be eligible for closing — completing a dead row while the account has no live step at all. + it('does nothing when the only pending step is merged-in', async () => { + const mergedPending = personalStep(ReviewStatus.IN_PROGRESS, -100); + await run(completeUser([mergedPending])); + + expect(kycStepRepo.update).not.toHaveBeenCalled(); + expect(mergedPending.status).toBe(ReviewStatus.IN_PROGRESS); + }); + it('leaves an already completed step untouched', async () => { const step = personalStep(ReviewStatus.COMPLETED); await run(completeUser([step])); diff --git a/src/subdomains/generic/kyc/services/kyc.service.ts b/src/subdomains/generic/kyc/services/kyc.service.ts index ec65c20585..07d1b5addd 100644 --- a/src/subdomains/generic/kyc/services/kyc.service.ts +++ b/src/subdomains/generic/kyc/services/kyc.service.ts @@ -661,27 +661,27 @@ export class KycService { * The verdict comes from the most recent SETTLED step, not from the whole history: a rejection the user has * since remedied ends in a completed step, and that account must stay eligible. * - * CANCELED alone cannot stand for "remedied": `initiateStep` cancels the previous COMPLETED step (:1359) but - * also cancels a merely PENDING one (:1349), so both a remediation and an untouched retry end up CANCELED. - * `result` separates them durably — `complete()` writes it and `cancel()` leaves it alone, while a step - * cancelled while still pending never had one. A result-less cancellation is therefore not evidence of - * anything and is skipped, so the FAILED step behind it still decides. + * CANCELED alone cannot stand for "remedied": `initiateStep` cancels the previous COMPLETED step in its + * PERSONAL_DATA branch, but its generic pending-step cancel also fires on a merely IN_PROGRESS one, so both a + * remediation and an untouched retry end up CANCELED. `KycStep.hasSettledVerdict` separates them — a + * cancellation only counts once the step had completed, which `result` records durably. */ async completeSatisfiedPersonalDataStep(userData: UserData): Promise { // The caller's UserData is loaded for its own flow and need not carry `kycSteps`; reload so the step // lookup never reads an undefined relation (same pattern as checkDfxApproval). const user = await this.userDataService.getUserData(userData.id, { kycSteps: true }); - const steps = user.getStepsWith(KycStepName.PERSONAL_DATA); + // The account's OWN chain only. A merge seeds the slave's rows 100 below the floor, so merged-in history + // is always negative while own rows start at 0 — and merge batches are ordered chronologically only within + // a batch, so ranking across them would let an older batch outrank a newer rejection. + const steps = user.getStepsWith(KycStepName.PERSONAL_DATA).filter((s) => s.sequenceNumber >= 0); + const lastSettled = Util.maxObj( - steps.filter((s) => !s.isInProgress && !(s.isCanceled && !s.result)), + steps.filter((s) => s.hasSettledVerdict), 'sequenceNumber', ); if (lastSettled?.isFailed) return; - // Highest sequence, not the first match: legacy merged-in accounts can carry more than one IN_PROGRESS - // step, and the merged-in ones sit at negative sequence numbers — `find` would close a dead step and - // leave the live one open. const kycStep = Util.maxObj( steps.filter((s) => s.isInProgress), 'sequenceNumber', From 9eee9cde355fb24c107be228aef83b0c6847a4aa Mon Sep 17 00:00:00 2001 From: Daniel Padrino Date: Fri, 31 Jul 2026 23:22:27 -0300 Subject: [PATCH 6/6] fix(kyc): treat a revoked completion as a rejection, not as a verdict Fourth review pass, second finding. restartStep revokes an outcome without erasing it: it calls fail(undefined, ...) and setResult(undefined) keeps the existing value, so a completed-then-restarted step still carries a stale result. Cancelling that row afterwards - a plausible support cleanup - made it read as a clean completion, and the retry was auto-completed with the rejected data. Excluding such a row from the settled set does not fix it: with no settled row left nothing blocks either. The verdict itself has to carry the signal, so KycStep.isRejected reads the durable RESTARTED_STEP marker alongside the status and stays authoritative after the status has moved past FAILED. --- .../generic/kyc/entities/kyc-step.entity.ts | 11 +++++++ .../services/__tests__/kyc.service.spec.ts | 32 ++++++++++++++++++- .../generic/kyc/services/kyc.service.ts | 6 ++-- 3 files changed, 46 insertions(+), 3 deletions(-) diff --git a/src/subdomains/generic/kyc/entities/kyc-step.entity.ts b/src/subdomains/generic/kyc/entities/kyc-step.entity.ts index 1fd3101f46..75cb6d727a 100644 --- a/src/subdomains/generic/kyc/entities/kyc-step.entity.ts +++ b/src/subdomains/generic/kyc/entities/kyc-step.entity.ts @@ -6,6 +6,7 @@ import { UserData } from '../../user/models/user-data/user-data.entity'; import { KycLevel, KycType, UserDataStatus } from '../../user/models/user-data/user-data.enum'; import { IdentDocumentType, IdentResultData, IdentType } from '../dto/ident-result-data.dto'; import { IdNowResult } from '../dto/ident-result.dto'; +import { KycError } from '../dto/kyc-error.enum'; import { ManualIdentResult } from '../dto/manual-ident-result.dto'; import { KycSessionInfoDto } from '../dto/output/kyc-info.dto'; import { IdDocTypeMap, ReviewAnswer, SumsubResult } from '../dto/sum-sub.dto'; @@ -216,10 +217,20 @@ export class KycStep extends IEntity { // Whether this row records an outcome. A cancellation only does so if the step had completed first: // `complete()` writes `result` and `cancel()` leaves it untouched, so a cancelled step without one was // never satisfied — it was merely superseded, and says nothing about the attempt it replaced. + // get hasSettledVerdict(): boolean { return !this.isInProgress && !(this.isCanceled && !this.result); } + // Whether this step's outcome was rejected rather than accepted. `restartStep` revokes an outcome without + // erasing it — it calls `fail(undefined, …)` and `setResult(undefined)` keeps the existing value — so a + // completed-then-restarted row still carries a stale `result` and, once cancelled, would otherwise read as a + // clean completion. The RESTARTED_STEP marker survives both writes, so it stays authoritative after the + // status has moved on. + get isRejected(): boolean { + return this.isFailed || (this.comment?.split(';').includes(KycError.RESTARTED_STEP) ?? false); + } + update( status: ReviewStatus, result?: KycStepResult, diff --git a/src/subdomains/generic/kyc/services/__tests__/kyc.service.spec.ts b/src/subdomains/generic/kyc/services/__tests__/kyc.service.spec.ts index 3d5f10bf41..6209abcfa0 100644 --- a/src/subdomains/generic/kyc/services/__tests__/kyc.service.spec.ts +++ b/src/subdomains/generic/kyc/services/__tests__/kyc.service.spec.ts @@ -806,13 +806,14 @@ describe('KycService completeSatisfiedPersonalDataStep', () => { let kycStepRepo: jest.Mocked; let userDataService: jest.Mocked; - const personalStep = (status: ReviewStatus, sequenceNumber = 0, result?: string): KycStep => + const personalStep = (status: ReviewStatus, sequenceNumber = 0, result?: string, comment?: string): KycStep => Object.assign(new KycStep(), { id: 2 + sequenceNumber, name: KycStepName.PERSONAL_DATA, status, sequenceNumber, result, + comment, }); // Every field in `requiredKycFields` for a personal account, so `isDataComplete` is true. @@ -930,6 +931,35 @@ describe('KycService completeSatisfiedPersonalDataStep', () => { expect(mergedPending.status).toBe(ReviewStatus.IN_PROGRESS); }); + // The shape carried by a number of merged prod accounts: the live chain is already COMPLETED at sequence 0 + // and only a merged-in leftover is still pending. Closing that dead row would stamp a COMPLETED verdict and + // a step log onto history inherited from a merged-away account, and leave two COMPLETED rows behind. + it('does nothing when only a merged-in step is pending and the live step is already completed', async () => { + const mergedPending = personalStep(ReviewStatus.IN_PROGRESS, -102); + const live = personalStep(ReviewStatus.COMPLETED, 0, '{"firstname":"Erika"}'); + await run(completeUser([mergedPending, live])); + + expect(kycStepRepo.update).not.toHaveBeenCalled(); + expect(mergedPending.status).toBe(ReviewStatus.IN_PROGRESS); + }); + + // restartStep calls fail(undefined, …) and setResult(undefined) keeps the existing value, so a + // completed-then-restarted row still carries a stale result. Cancelling it afterwards must not read as a + // clean completion — the RESTARTED_STEP marker survives both writes and says the outcome was withdrawn. + it('leaves the chain alone when a restarted step was later cancelled but kept its stale result', async () => { + const withdrawn = personalStep( + ReviewStatus.CANCELED, + 0, + '{"firstname":"Erika"}', + 'PersonalDataNotMatching;RestartedStep', + ); + const pending = personalStep(ReviewStatus.IN_PROGRESS, 1); + await run(completeUser([withdrawn, pending])); + + expect(kycStepRepo.update).not.toHaveBeenCalled(); + expect(pending.status).toBe(ReviewStatus.IN_PROGRESS); + }); + it('leaves an already completed step untouched', async () => { const step = personalStep(ReviewStatus.COMPLETED); await run(completeUser([step])); diff --git a/src/subdomains/generic/kyc/services/kyc.service.ts b/src/subdomains/generic/kyc/services/kyc.service.ts index 07d1b5addd..6ee643b547 100644 --- a/src/subdomains/generic/kyc/services/kyc.service.ts +++ b/src/subdomains/generic/kyc/services/kyc.service.ts @@ -664,7 +664,9 @@ export class KycService { * CANCELED alone cannot stand for "remedied": `initiateStep` cancels the previous COMPLETED step in its * PERSONAL_DATA branch, but its generic pending-step cancel also fires on a merely IN_PROGRESS one, so both a * remediation and an untouched retry end up CANCELED. `KycStep.hasSettledVerdict` separates them — a - * cancellation only counts once the step had completed, which `result` records durably. + * cancellation only counts once the step had completed, which `result` records durably — and + * `KycStep.isRejected` reads the verdict itself, so a completion later revoked by a restart still blocks + * even after its status has moved past FAILED. */ async completeSatisfiedPersonalDataStep(userData: UserData): Promise { // The caller's UserData is loaded for its own flow and need not carry `kycSteps`; reload so the step @@ -680,7 +682,7 @@ export class KycService { steps.filter((s) => s.hasSettledVerdict), 'sequenceNumber', ); - if (lastSettled?.isFailed) return; + if (lastSettled?.isRejected) return; const kycStep = Util.maxObj( steps.filter((s) => s.isInProgress),