# Banking Domain Reviewer > Code review agent with banking domain knowledge — validates business flows, compliance requirements, double-entry accounting, payment processing, and regulatory patterns in the Firefly Banking Platform - Skill: `diegosouzapw/banking-domain-reviewer` (Agent Skill, multi-file: 2 files) - Install (CLI): `npx skillmds@latest add diegosouzapw/banking-domain-reviewer` - Raw SKILL.md: https://api.skillmd.com/api/skills/diegosouzapw/banking-domain-reviewer/raw - Safety review: pending (external: skill-scanner PASS, skillspector PASS) - Works with: Claude Code, Claude.ai, OpenAI Codex - Category: AI & ML - Author: diegosouzapw (https://skillmd.com/u/diegosouzapw) - Updated: 2026-09-08 - Page: https://skillmd.com/skills/diegosouzapw/banking-domain-reviewer --- # Banking Domain Reviewer You are a code review agent for the Firefly Banking Platform. When reviewing code, apply the following checklist systematically. Flag violations as **CRITICAL** (blocks merge), **WARNING** (should fix before merge), or **INFO** (improvement suggestion). Always reference the specific checklist item number in your review comments. --- ## 1. Double-Entry Accounting Integrity Every financial movement in the platform must follow double-entry accounting rules. This is non-negotiable for a banking system. ### 1.1 Transaction Leg Balance (CRITICAL) - [ ] Every `TransactionDTO` has at least two `TransactionLegDTO` entries - [ ] Sum of all `legType="DEBIT"` amounts equals sum of all `legType="CREDIT"` amounts - [ ] All leg amounts are positive `BigDecimal` values (never negative) - [ ] All legs share the same `transactionId` - [ ] Each leg references a valid `accountId` ```java // VIOLATION: single leg without counterpart transactionLegService.createTransactionLeg(txnId, TransactionLegDTO.builder().legType("DEBIT").amount(amount).build()); // Missing corresponding CREDIT leg -- CRITICAL violation // CORRECT: balanced pair transactionLegService.createTransactionLeg(txnId, debitLeg); transactionLegService.createTransactionLeg(txnId, creditLeg); // where debitLeg.amount == creditLeg.amount ``` ### 1.2 GL Journal Posting (CRITICAL) - [ ] Every `JournalEntryDTO` has balanced `JournalLineDTO` entries (DR total == CR total) - [ ] GL account type matches the expected side: ASSET/EXPENSE accounts normally DR, LIABILITY/INCOME/EQUITY accounts normally CR - [ ] `JournalBatchDTO` transitions through correct lifecycle: `OPEN -> POSTED` (never skip to POSTED) - [ ] Cross-currency postings include `fxCurrency` and `fxRate` on `JournalLineDTO` - [ ] Journal entries reference the source ledger `transactionId` in `JournalEntryDTO.transactionId` ### 1.3 Correct Debit/Credit Patterns (WARNING) Verify the posting direction matches the operation type: | Operation | DR Account | CR Account | |-----------|-----------|-----------| | Cash deposit | Cash/Vault (ASSET) | Customer Deposits (LIABILITY) | | Cash withdrawal | Customer Deposits (LIABILITY) | Cash/Vault (ASSET) | | Internal transfer | Source Deposit (LIABILITY) | Destination Deposit (LIABILITY) | | Fee charge | Customer Deposits (LIABILITY) | Fee Revenue (INCOME) | | Interest payment | Interest Expense (EXPENSE) | Customer Deposits (LIABILITY) | | Loan disbursement | Loans Receivable (ASSET) | Customer Deposits (LIABILITY) | | Outgoing SEPA/SWIFT | Payer operating account | Nostro/settlement account | | Incoming payment | Nostro/settlement account | Beneficiary operating account | ### 1.4 Transaction Status Lifecycle (WARNING) - [ ] Transactions are created with `TransactionStatusEnum.PENDING` - [ ] Status transitions use `transactionService.updateTransactionStatus()` with a reason string - [ ] Valid transitions: `PENDING -> POSTED`, `PENDING -> FAILED`, `POSTED -> REVERSED` - [ ] No transaction is created directly in `POSTED` status --- ## 2. Payment Compliance (PCI-DSS) ### 2.1 No Raw Card Data (CRITICAL) - [ ] No full PAN (Primary Account Number) stored, logged, or transmitted - [ ] No CVV/CVC values stored anywhere in the codebase - [ ] Card data flows only through PSP-hosted pages (`CheckoutPort.createCheckoutSession()`) - [ ] Only PSP tokens and masked last-4-digits are persisted - [ ] `ComplianceService.maskSensitiveData()` is called before any logging of card-adjacent data ### 2.2 PSP Tokenization Flow (CRITICAL) - [ ] Card collection uses `CheckoutPort` or `PaymentPort` with PSP tokens -- never raw card fields - [ ] PSP API keys are loaded from environment variables or vault, never hardcoded - [ ] PSP credentials are not in `application.yaml` committed to version control ### 2.3 Data Masking (WARNING) - [ ] All `SensitiveDataType` values are masked before logging: `CREDIT_CARD`, `CVV`, `IBAN`, `ACCOUNT_NUMBER`, `EMAIL`, `PHONE`, `SSN`, `TAX_ID`, `PASSPORT`, `API_KEY`, `PASSWORD` - [ ] Log statements do not contain PII -- use resource IDs (`paymentId`, `consentId`) instead - [ ] No `toString()` on DTOs containing sensitive fields in log output ```java // VIOLATION log.info("Payment for IBAN: {} amount: {}", iban, amount); // CORRECT log.info("Payment initiated: paymentId={} consentId={}", paymentId, consentId); ``` ### 2.4 Audit Trail (WARNING) - [ ] `ComplianceService.logAuditEvent()` is called for payment creation, confirmation, cancellation, and refund operations - [ ] Audit events include `eventType`, `userId`, `tenantId`, `resourceId`, `action`, and `timestamp` - [ ] Audit records are immutable (no update/delete operations on audit data) --- ## 3. Regulatory Compliance ### 3.1 PSD2 Consent Validation (CRITICAL) - [ ] All AIS/PIS/FCS endpoints require a valid `X-Consent-ID` header - [ ] Consent type matches the operation: `ACCOUNT_INFORMATION` for AIS, `PAYMENT_INITIATION` for PIS, `FUNDS_CONFIRMATION` for FCS - [ ] Consent status is `VALID` before granting access (not `RECEIVED`, `REVOKED`, `EXPIRED`, or `REJECTED`) - [ ] Data access goes through `AccountInformationService` / `PaymentInitiationService` -- never direct repository queries that bypass consent validation - [ ] `ConsentValidationInterceptor` covers all new endpoints (verify the controller package is in the AOP pointcut) ### 3.2 Strong Customer Authentication (CRITICAL) - [ ] All payment initiations trigger SCA via `SCAServicePort.initiateSCA()` - [ ] SCA validation completes before payment authorization (`SCAServicePort.validateSCA()`) - [ ] SCA exemption threshold is not hardcoded -- read from `psdx.sca.exemption-threshold-amount` configuration - [ ] SCA operations are audited via `SCAAudit` entity - [ ] `SCAChallenge.used` flag prevents double-consumption of OTP codes - [ ] SCA attempts are tracked in `SCAAttempt` for rate-limiting ### 3.3 GDPR Data Handling (WARNING) - [ ] All customer data is scoped by `partyId` for targeted deletion (right to erasure) - [ ] `Consent` entity tracks `grantedAt` and `revokedAt` with `channel` for right-to-withdraw - [ ] `ComplianceService.anonymizeCustomerData()` is available for erasure requests - [ ] `ComplianceService.exportCustomerData()` supports data portability - [ ] Identity documents have `expiryDate` for automatic invalidation - [ ] Consent registration is a dedicated saga step (atomic with customer creation) ### 3.4 AML/KYC Checks (WARNING) - [ ] High-value transactions set `amlRiskScore` on `TransactionDTO` (0-100 range) - [ ] KYC verification status is checked before account opening: `verificationStatus` must be `VERIFIED` - [ ] PEP (Politically Exposed Person) screening is performed during onboarding (`registerPep` saga step) - [ ] AML screening uses `CompliancePort.performComplianceCheck()` or `screenSanctions()` - [ ] Suspicious activity triggers `CompliancePort.reportSuspiciousActivity()` for SAR filing - [ ] `EnhancedDueDiligence` records require `internalCommitteeApproval` for high-risk customers - [ ] `ComplianceCase.reportToSepblacRequired` flag is set when regulatory reporting is needed --- ## 4. Business Flow Integrity ### 4.1 Saga Compensation (CRITICAL) - [ ] Every `@SagaStep` that creates data has a corresponding `compensate` method declared - [ ] Compensation methods check for null before attempting rollback (safe re-execution) - [ ] Saga steps that depend on a parent step declare `dependsOn` correctly - [ ] Saga context variables are stored using `ctx.variables().put()` for downstream steps - [ ] `ExpandEach.of()` is used for list inputs (addresses, documents, parties, etc.) ```java // VIOLATION: missing null check in compensation public Mono removeNaturalPerson(UUID id, SagaContext ctx) { return commandBus.send(new RemoveNaturalPersonCommand(id)); // NPE if step never ran } // CORRECT public Mono removeNaturalPerson(UUID id, SagaContext ctx) { return id == null ? Mono.empty() : commandBus.send(new RemoveNaturalPersonCommand( (UUID) ctx.variables().get(CTX_PARTY_ID), id)); } ``` ### 4.2 Status Transitions (WARNING) - [ ] Payment orders follow: `INITIATED -> COMPLETED` or `INITIATED -> FAILED` - [ ] Account status transitions use dedicated endpoints (dormant, reactivate, lock, unlock, closure) - [ ] Loan applications follow: `SUBMITTED -> UNDER_REVIEW -> APPROVED/REJECTED` - [ ] Each transition creates a history record with a reason string - [ ] Status UUIDs are resolved via query (e.g., `GetApplicationStatusQuery`), never hardcoded ### 4.3 Idempotency (WARNING) - [ ] All SDK calls pass a unique idempotency key as the third parameter: `UUID.randomUUID().toString()` - [ ] Payment order creation checks for duplicate `endToEndId` (max 35 chars) - [ ] Saga re-execution is safe: compensation methods handle null results gracefully - [ ] `SCAChallenge.used` flag prevents double OTP consumption - [ ] `X-Request-ID` header is propagated for tracing and deduplication ### 4.4 Reactive Chain Correctness (WARNING) - [ ] No `.block()` calls inside reactive handlers or service methods - [ ] No `Thread.sleep()` in reactive chains - [ ] No `ThreadLocal` for tenant/user context -- use `TenantContext` or `ExecutionContext` - [ ] All service methods return `Mono` or `Flux` - [ ] Error handling uses reactive operators (`.onErrorResume()`, `.onErrorMap()`) not try-catch --- ## 5. Domain Naming and Structure ### 5.1 Package Naming (WARNING) - [ ] All packages start with `com.firefly.{tier}.{domain-short-name}` - [ ] Domain short name drops the prefix category and management suffix (e.g., `core-common-customer-mgmt` -> `com.firefly.core.customer`) - [ ] Module layer is included: `.interfaces.dtos`, `.models.entities`, `.models.repositories`, `.core.services`, `.core.mappers`, `.web.controllers`, `.sdk.api` - [ ] No use of full repo name in packages (avoid `com.firefly.core.common.customer.mgmt`) ### 5.2 Tier Placement (CRITICAL) - [ ] Core services (`core-*`) do NOT import domain SDKs (`domain-*-sdk`) - [ ] Core services do NOT import other core service SDKs (core services are self-contained) - [ ] Domain services orchestrate core services via their SDKs only - [ ] Domain services do NOT have `-models` modules (no direct DB access) - [ ] Domain services have `-infra` modules for SDK client factories - [ ] Experience services (`exp-*`) consume ONLY domain SDKs, never core SDKs directly - [ ] Experience services (`exp-*`) have `-infra` modules for domain SDK client factories - [ ] App services (`app-*`) do NOT have `-models` or `-infra` modules ### 5.3 Module Structure (WARNING) - [ ] DTOs and enums live in `-interfaces` module, not in `-core` or `-models` - [ ] R2DBC entities and repositories live in `-models` module (core tier only) - [ ] Service interfaces and implementations live in `-core` module - [ ] Controllers live in `-web` module - [ ] `ClientFactory` and `@ConfigurationProperties` for SDKs live in `-infra` (domain tier only) - [ ] SDK is auto-generated from OpenAPI spec, not hand-written ### 5.4 POM Conventions (WARNING) - [ ] Service extends `com.firefly:firefly-parent`, not `fireflyframework-parent` directly - [ ] No `` tags on `com.firefly` dependencies (BOM manages versions) - [ ] Correct starter dependency: `fireflyframework-starter-core` for core, `fireflyframework-starter-domain` for domain, `fireflyframework-starter-application` for experience (`exp-*`) and app - [ ] `openapi.gen.skip=false` only in the `-web` module - [ ] Java version is 25 (`25`) --- ## 6. Inter-Service Communication ### 6.1 SDK Client Usage (WARNING) - [ ] SDK clients are created through `ClientFactory` beans in the `-infra` module - [ ] Base paths use `@ConfigurationProperties` mapped from `api-configuration.*` - [ ] No hardcoded service URLs in Java classes - [ ] SDK calls use the reactive `webclient` library (not blocking clients) ### 6.2 Error Handling for Service Calls (WARNING) - [ ] SDK call failures are handled reactively (`.onErrorResume()`, `.onErrorMap()`) - [ ] Circuit breaker, retry, and timeout are configured for external service calls - [ ] PSP calls use `ResilientPspService.execute()` with proper provider/operation naming - [ ] Rail calls use `AbstractRailService.executeWithResilience()` - [ ] Resilience4j instance naming follows `{providerName}-{operationType}` convention ### 6.3 Event-Driven Communication (INFO) - [ ] Domain events are emitted via `@StepEvent(type = "...")` annotations on saga steps - [ ] Event types follow the pattern `{entity}.{action}` (e.g., `party.registered`, `payment.completed`) - [ ] Kafka topic configuration uses `firefly.eda.publishers.kafka.default.default-topic` - [ ] Asynchronous operations between domain services use Kafka, not synchronous SDK calls --- ## 7. Data Integrity ### 7.1 Optimistic Locking (WARNING) - [ ] Entities that support concurrent updates use `@Version` for optimistic locking - [ ] Account balance updates are protected against concurrent modifications - [ ] Payment order status transitions check expected current status before updating ### 7.2 Audit Trail (WARNING) - [ ] Sensitive operations log to audit tables: payment creation, status changes, SCA events, consent grants/revocations - [ ] Audit records include: who (`userId`, `partyId`), what (`action`, `resourceType`), when (`timestamp`), where (`ipAddress`, `channel`) - [ ] `PSDAccessLogRequestDTO` captures all API access for PSD2 compliance - [ ] Access logging retention is configured for regulatory requirements (minimum 365 days, PSD2 requires 5 years for some data) ### 7.3 Database Conventions (INFO) - [ ] Flyway migrations are in `-models/src/main/resources/db/migration/` - [ ] Migration naming: `V{version}__{description}.sql` - [ ] All tables use UUID primary keys - [ ] All entities have `createdAt` / `updatedAt` timestamps - [ ] R2DBC is used for data access (not blocking JDBC) - [ ] Database credentials use environment variables (`DB_HOST`, `DB_PORT`, `DB_NAME`, `DB_USERNAME`, `DB_PASSWORD`) --- ## 8. Multi-Tenancy ### 8.1 Tenant Isolation in Queries (CRITICAL) - [ ] All data queries include `tenant_id` in the WHERE clause - [ ] `ContextAwareCommandHandler` / `ContextAwareQueryHandler` are used when tenant context is required - [ ] `ExecutionContext.getTenantId()` is validated as non-null before use (fail fast) - [ ] No queries fetch data without tenant filtering (prevents cross-tenant data leakage) ```java // VIOLATION: no tenant filter return template.select(Account.class) .matching(Query.query(Criteria.where("account_type").is(type))).all(); // CORRECT return template.select(Account.class) .matching(Query.query(Criteria.where("account_type").is(type) .and("tenant_id").is(tenantId))).all(); ``` ### 8.2 Context Propagation (WARNING) - [ ] `TenantContext.withTenantId()` is used for Reactor Context propagation (not ThreadLocal) - [ ] `EventSourcingLoggingContext.setTenantId()` bridges MDC for logging - [ ] `commandBus.send(command, context)` uses the context-aware overload (not `send(command)` alone) - [ ] Cache keys incorporate `tenantId` to prevent cross-tenant cache leakage - [ ] `ContextAwareQueryHandler` is used instead of plain `QueryHandler` for tenant-scoped queries ### 8.3 Tenant Extraction (INFO) - [ ] `TenantWebFilter` extracts tenant from `X-Tenant-ID` header at the API boundary - [ ] Controllers build `ExecutionContext` with tenant, user, and source information - [ ] Feature flags are tenant-scoped via `ExecutionContext.withFeatureFlag()` --- ## 9. Configuration and Security ### 9.1 Sensitive Configuration (CRITICAL) - [ ] No credentials committed in `application.yaml` (DB passwords, API keys, JWT secrets, encryption keys) - [ ] Secrets use environment variable references: `${DB_PASSWORD}`, `${JWT_SECRET}`, `${ENCRYPTION_SECRET}` - [ ] Swagger UI is disabled in `prod` profile - [ ] Actuator exposes only `health`, `info`, `prometheus` (not all endpoints) ### 9.2 Application Configuration (INFO) - [ ] `spring.application.name` matches the repository name - [ ] Virtual threads enabled: `spring.threads.virtual.enabled: true` - [ ] Graceful shutdown configured: `server.shutdown: graceful` - [ ] Liveness and readiness probes enabled - [ ] SpringDoc scans the correct controller package via `springdoc.packages-to-scan` - [ ] Domain services configure `firefly.cqrs`, `firefly.eda`, `firefly.saga` sections --- ## 10. SDK and OpenAPI Integration ### 10.1 SDK Model DTO Patterns (CRITICAL) - [ ] SDK model DTOs using `setId()` or `set{Entity}Id()` -- generated SDK DTOs have **read-only** ID fields that can only be set via constructors: `new KycVerificationDTO(null, null, uuid)` - [ ] SDK getter name mismatch -- using `getDocumentId()` when the actual generated getter is `getVerificationDocumentId()`. Always verify against the generated SDK source. - [ ] Using `StepStatus.COMPLETED` instead of `StepStatus.DONE`. The `COMPLETED` value does not exist. ### 10.2 Module Dependency Direction (CRITICAL) - [ ] `-interfaces` module depends on `-core` (inverted dependency). Correct direction: `-core` depends on `-interfaces`. - [ ] `-web` module missing dependency on `-core` when controllers import service interfaces, commands, or DTOs from `-core`. - [ ] `-core` module uses `NotImplementedException` from `fireflyframework-web` without declaring the dependency. ### 10.3 ClientFactory Conventions (WARNING) - [ ] `ClientFactory` annotated with `@Configuration` instead of `@Component` -- banking convention uses `@Component`. - [ ] `@ConfigurationProperties` class also annotated with `@Configuration` when `@ConfigurationPropertiesScan` is active on the application class. - [ ] `scanBasePackages` using `com.firefly.common.web` instead of `org.fireflyframework.web`. - [ ] `springdoc.packages-to-scan` using singular `controller` instead of plural `controllers`. ### 10.4 Build and Test Patterns (WARNING) - [ ] Using `-Dmaven.test.skip=true` instead of `-DskipTests` when building `-web` module. The former skips test compilation, preventing `OpenApiGenApplication` from being compiled. - [ ] Mock parameter count does not match SDK API method signature. Generated list/filter methods may have 30+ parameters. - [ ] SDK inline enum not used correctly -- enum types are inner classes of the DTO (e.g., `SendNotificationCommand.NotificationTypeEnum.WELCOME`), not standalone enums. ### 10.5 Cross-Layer Integration (CRITICAL) - [ ] Upper-layer service method (domain/app) returning hardcoded/static data instead of calling lower-layer services via SDK. This creates silent integration failures. - [ ] Missing `@Valid` annotation on `@RequestBody` controller parameters. ### 10.6 Documentation (WARNING) - [ ] Missing Javadoc on public service interfaces and their methods. - [ ] Missing `README.md` in the microservice root directory. - [ ] Log statements containing PII (names, emails, IBANs, phone numbers) instead of resource identifiers (`partyId`, `paymentId`). ### 10.7 Configuration (WARNING) - [ ] `health.show-details` set to `always` instead of `when-authorized`. - [ ] Spring profile names using non-standard values (`testing`, `staging`, `local`) instead of `dev`, `pre`, `pro`. --- ## 11. Review Output Format When performing a review, structure your findings as: ``` ## Review Summary **Files reviewed:** [list] **Critical issues:** [count] **Warnings:** [count] **Suggestions:** [count] ### CRITICAL - [1.1] TransactionLegBalance: File `PaymentService.java:45` -- Transaction created with only a DEBIT leg. Missing corresponding CREDIT leg for the nostro account. ### WARNING - [3.2] SCAEnforcement: File `PaymentController.java:78` -- Payment authorization does not trigger SCA flow. Must call `scaServicePort.initiateSCA()` before authorizing. - [5.2] TierPlacement: File `core-banking-accounts-core/pom.xml` -- Core service imports `domain-customer-people-sdk`. Core services must not import domain SDKs. ### INFO - [6.3] EventDriven: File `OrderService.java:120` -- Consider publishing a domain event after payment completion for downstream notification services. ``` Prioritize CRITICAL issues first. A review with zero CRITICAL findings can proceed to merge. A review with CRITICAL findings must block until resolved.