fix(kyc): keep status and sequence number on the financial data response

[joshuakrueger-dfx]

Not symptom-driven: the "status does not change after submitting the financial information"
report is unresolved — the raw response body was requested and never received. This PR does not
claim to close it. What is proven is a contract break found while tracing that path.
Scale: deterministic on every call, across 13 controller routes — sequenceNumber was
missing from every KycStep response built after a KycStep.update() call that omitted it, and
status additionally from the financial-data one. How many clients trip over it is not measured
and not claimed.
Smaller fix considered: this PR is the smallest fix. The larger one (#4540, now draft) repairs
KycStep.update() generically and is not merged for the reason under Why not one change inside update().

Problem

PUT /kyc/data/financial/:id answers 200 with a body that has no status and no
sequenceNumber, although KycStepBase declares both required — the endpoint violates its own
Swagger contract.

updateFinancialData calls kycStep.update(undefined, data.responses). KycStep.update() builds
{ status, result, comment, sequenceNumber } and runs Object.assign(this, update), which copies
undefined onto the entity. KycStepMapper.toStepBase then maps StepMap[undefined] →
undefined, and both fields drop out of the JSON. The DB row is unaffected — TypeORM strips
undefined before the SET clause — the loss is in memory, on the object that becomes the response.

For our own frontends the visible behaviour is the same either way: both go through
isStepDone(result) in @dfx.swiss/core, which is false with the field absent and false with
inProgress present. A third-party client generated from the contract is a different matter.

The same wipe hits three further call sites, so sequenceNumber was missing from 13 routes in
total — the table below lists them.

Solution

KycStep.update() is the only entity method that puts sequenceNumber into the partial, so it is
the only one that can wipe it. Four call sites omitted the argument and mapped the result into a
response; each now passes the current value through. The financial-data one additionally lost
status and is fixed differently, because there the status was omitted too:

Call site	Fix	Reached from
kyc.service.ts:963 updateFinancialData	kycStep.inProgress(data.responses)	1 route (PUT /kyc/data/financial/:id)
kyc.service.ts:820 updateFileData	pass kycStep.sequenceNumber	6 routes (owner directory, residence permit, statutes, authority, …)
kyc.service.ts:1057 updateKycStepAndLog	pass kycStep.sequenceNumber	5 routes, via updateKycStep (2), updateBeneficialOwnerData, updateOperationActivityData, updatePaymentData
kyc.service.ts:1709, :1721, :1724, :1728 reviewNationalityData	pass kycStep.sequenceNumber	1 route, via updateNationalityStep

For the financial path:

-await this.kycStepRepo.update(...kycStep.update(undefined, data.responses));
+await this.kycStepRepo.update(...kycStep.inProgress(data.responses));

inProgress() is the pattern the other result writes on a running step already use
(kyc.service.ts:350, :1140, :1162). It never puts sequenceNumber into the partial, so the
value stays on the entity instead of being wiped. It forces no status transition here:
getPendingStepOrThrow throws unless the step isInProgress, i.e. status === IN_PROGRESS already.

Why not one change inside update()

Filtering the omitted arguments inside update() would cover all four at once, but that method has
11 production call sites, several of them in mergeUserData. Two consequences, both unwanted:
repairing the status wipe makes merged slave steps visible to checkDfxApproval, which writes old
steps to OUTDATED and sends a kycStepReminder mail to the master — customer-visible, untested,
unmeasured. And pendingRecommendation.update(ReviewStatus.COMPLETED) (user-data.service.ts:1631)
omits sequenceNumber as well, so even the narrower variant would change in-memory behaviour on the
merge path. Fixing the call sites keeps every one of those untouched: kyc-step.entity.ts and
user-data.service.ts are not in this diff at all. #4540 (draft) holds the full analysis of that
path, including the measurement (791 accounts with a completed video ident step, 27 without
identificationType = VideoId, 2 of them touched since 2024-11-11).

The one remaining update() without the argument is checkDfxApproval (kyc.service.ts:438,
expiredStep.update(OUTDATED, …)). It is deliberately left alone: no toStepBase follows it, so no
response is affected.

One difference not visible in the diff: the financial call previously also wrote comment (via
addComment(undefined), which returns the existing comment, or '' when there is none).
inProgress() does not touch comment. Every consumer reads it through ?.split(';') or a
truthiness check, so '' and NULL are indistinguishable to all of them — no behaviour change, and
it stops a pointless NULL → '' write.

Coverage

CONTRIBUTING.md requires every touched file at 100% on all four metrics and pinned in the same PR.
src/subdomains/generic/kyc/services/kyc.service.ts is pinned in PINNED_LOGIC.

Measured on this revision with the coverage-gate compile settings (jest.coverage-gate.config.js,
tsconfig.coverage.json) on the five kyc.service*.spec.ts files:

File	Statements	Branches	Functions	Lines
src/subdomains/generic/kyc/services/kyc.service.ts	100% (846/846)	100% (419/419)	100% (147/147)	100% (759/759)

To reach 100% branches, two unreachable or redundant fragments in completeIdent /
getIdentCheckErrors are dropped:

• The else if (nationality) / NATIONALITY_MISSING fail after the outer if already required
  nationality — that fail branch could not run.
• The second Util.includesSameName argument order. The helper is the word-set intersection, so
  the two directions are the same.

Tests

Four original response tests in kyc.service.spec.ts — one per call site — asserting on the
response of the service method rather than on the entity, each pinning a concrete
sequenceNumber (7, 4, 5) instead of toBeDefined(), so a wrong value fails too.

Four additional spec files cover the rest of kyc.service.ts so the file can be pinned.

On this revision, under the coverage-gate compile:

Probe	Result
five kyc.service*.spec.ts files	5 suites, 320 tests passed; file at 100% on all four metrics
financial fix mutated back to update(undefined, responses)	the original financial response test fails (Expected InProgress, Received undefined)
lint on the changed TypeScript files	empty output
type-check	exit 0
format:check	exit 0

CI on head 73238d0a6 (all required and reported checks): Build and checks, Test shards 1–3, Coverage, Coverage ratchet, Read-path projections, Full-stack E2E, review, CodeQL, Analyze — all SUCCESS. Shard totals: 177/176/175 suites passed; 3868/3924/3709 tests passed.

Scope

Not in this PR: any change to KycStep.update(), to mergeUserData, or to the video-ident branch
there — all three stay exactly as they are on develop, and neither file appears in the diff. No DTO
change, no complete/missingFields extension (that was #4426, closed), no change to
FinancialService.

Not verified: no end-to-end run against the live endpoint — the tests drive the service with a
mocked repository. Full-stack E2E on this head is green, but that harness does not exercise
PUT /kyc/data/financial/:id. And the number that would carry the necessity is still missing: it
is not established that any client currently fails because of the missing fields. What is
established is that the response contradicts the declared contract.

Final pass (73238d0):
Coherent: the four call-site writes restore the required response fields; the extra specs and
the PINNED_LOGIC entry exist only because CONTRIBUTING requires 100% on the whole touched file;
the two production deletions drop an unreachable NATIONALITY_MISSING fail and a symmetric
includesSameName call so the 100% branch count is honest.
Nothing extra: KycStep.update(), mergeUserData, and the leftover checkDfxApproval
update(OUTDATED) stay as they are on develop (#4540). The ratchet listed 97 other unpinned
complete files; they are not this PR's files and are not pinned here.
Sources closed: TaprootFreak CHANGES_REQUESTED on c0b4b07b (100% four metrics, pin,
per-file numbers) — done on this head. Mara APPROVED c0b4b07b; this head needs a fresh review. #4893 test hygiene left as the stated non-blocking follow-up. The original “status does
not change” report is still not claimed closed.

[marassteiner]

EN: Working on this now — job dfxswiss__api__4669__pr-review.
DE: Ich arbeite jetzt daran — Auftrag dfxswiss__api__4669__pr-review.

[marassteiner]

Verdict: Approve

No merge blockers. The PR correctly restores required status / sequenceNumber fields on KYC step responses that were wiped in memory by KycStep.update() + Object.assign when optional args were omitted.

Summary

• Financial incomplete path: update(undefined, responses) → inProgress(responses) — does not put sequenceNumber into the partial, keeps IN_PROGRESS status (already guaranteed by getPendingStepOrThrow).
• Complete financial path: subsequent internalReview() also leaves sequenceNumber intact; response mapper still sees both fields.
• Other three response-producing call sites (updateFileData, updateKycStepAndLog, reviewNationalityData): pass kycStep.sequenceNumber as 4th arg (evaluated before Object.assign wipe).
• Deliberately does not change KycStep.update() itself — avoids merge-path / checkDfxApproval side effects (#4540 territory). Scope is minimal and appropriate.
• Four new unit tests pin concrete sequenceNumber (and status for financial) on the mapped response, not just the entity.

Merge blockers

None.

Follow-up findings (non-blocking)

Test-only hygiene from the conformity pass — tracked in #4893:

1. Import grouping for AccountType / KycStepStatus (kyc.service.spec.ts:24-25)
2. as any on uploadUserFile mock (kyc.service.spec.ts:908)
3. as any on nationality DTO fragment (kyc.service.spec.ts:1016-1017)

Optional coverage gaps (not defects): complete-financial response assert; nationality branches other than residence-permit; status asserts on non-financial paths.

Local verification

Check	Result
CI on PR (Build and checks, Test shards, Coverage, CodeQL, review)	All SUCCESS
npm ci	OK
npm run build	exit 0
npm test -- --testPathPattern=kyc/services/__tests__/kyc.service.spec	63/63 passed
ESLint on the two changed files	clean
Full API boot (node dist/src/main)	skipped — environmental

Local boot (Case 2 — outside this diff): process exits before Nest init with:

Error: Missing REALUNIT_W2W_GAS_LOW_BALANCE_THRESHOLD
  at dist/src/config/config.js (Configuration constructor)

Evidence this is not the PR:

• Diff only touches kyc.service.ts + its unit test file
• Failure is a pre-existing fail-loud config guard for RealUnit W2W gas (src/config/config.ts around the REALUNIT_W2W_GAS_LOW_BALANCE_THRESHOLD read)
• Review host has no full .env / secrets; the PR neither adds nor uses that variable

LOCAL_RUN_SKIPPED_ENV for full process boot; unit suite + build exercised the change successfully.

Recommendation

Approve and merge when ready. No request-changes items.

[TaprootFreak]

EN: Changes required: kyc.service.ts is not at 100% on the whole file and is not pinned in the coverage ratchet.
DE: Änderung zwingend erforderlich: kyc.service.ts ist nicht auf der ganzen Datei bei 100% und nicht im Coverage-Ratchet gepinnt.

Details

CONTRIBUTING.md § Test Coverage: every file a PR touches must reach 100% on all four metrics (branches, functions, lines, statements) on the whole file, and must be pinned in jest.coverage-gate.config.js in the same PR.

This PR touches src/subdomains/generic/kyc/services/kyc.service.ts and adds specs only for the changed response-field paths (updateFinancialData, updateFileData, updateKycStepAndLog, updateNationalityStep).

Gaps:

• jest.coverage-gate.config.js is not in the PR. kyc.service.ts is not pinned on this head.
• New tests for four methods do not bring this large existing service to 100%. Touching it makes the entire file the author's obligation. If that is not feasible here, cover it in a preparatory PR first, then land this fix.

E2E/handbook: not applicable (no new HTTP surface, no handbook artefacts). The unit/ratchet gap is sufficient to block.

Required:

1. Bring kyc.service.ts to 100% on all four metrics (measure with the coverage-gate compile settings; see docs/coverage-gate.md).
2. Add the path to PINNED_LOGIC in jest.coverage-gate.config.js in this PR.
3. State the per-file numbers in the PR description.