feat(infra): extract common modules to eliminate cross-service duplication (STA-202)#191
Merged
Merged
Conversation
…module (STA-202) Introduce Gradle buildSrc convention plugins (stablebridge.service, stablebridge.api-library, stablebridge.client-library) to eliminate ~1,800 lines of duplicated build configuration across 30 modules. Extract shared ApiError record into platform-api module, replacing 10 identical copies across service -api modules. Each service build.gradle.kts reduced from ~205 lines to 15-39 lines. Closes STA-202 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… (STA-202) Create platform-test module with shared test fixtures: - TestUtils (eqIgnoringTimestamps, eqIgnoring) — replaces 10 identical copies - HexagonalArchitectureRules (verifyAll) — replaces 10 identical ArchitectureTests - TestContainerSupport (postgres, kafka, redis factories + startAll/safeStop) — DRYs 10 AbstractIntegrationTests - TestSecurityConfig (permit-all) — replaces 4 identical test configs Migrate all 10 services to use shared test infrastructure. Net: +213 / -1,767 lines. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ture (STA-202) Create platform-infra module with shared runtime classes: - AbstractOutboxHandler — replaces 10 identical OutboxHandler implementations - AbstractOutboxEventPublisher — replaces 8 similar publishers with configurable key fields - BaseGlobalExceptionHandler — extracts 6 shared exception handlers from 10 services Each service now has thin subclasses with only domain-specific logic. Net: +160 / -1,082 lines. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…y (STA-202) Fix fiat-on-ramp errorCodePrefix "ON" → "OR" and payment-orchestrator errorCodePrefix "OR" → "PO" to match existing domain error codes. Remove unnecessary Redis container from payment-orchestrator ITs that was causing PostgreSQL container crashes. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
WalkthroughAdds shared platform modules (platform-api, platform-infra, platform-test), centralizes ApiError, global exception handling, outbox publishing/handling, and test container/utilities; converts many per-service Gradle scripts to stablebridge plugin presets and updates services to extend platform abstractions. Changes
Sequence Diagram(s)sequenceDiagram
participant Service as Service Code
participant Publisher as AbstractOutboxEventPublisher
participant Outbox as Outbox (DB)
participant Kafka as Kafka Topic
Service->>Publisher: publish(event)
activate Publisher
Publisher->>Publisher: resolveKey(event)
Publisher->>Outbox: schedule(event, key)
deactivate Publisher
Outbox->>Kafka: outbox poller publishes
Kafka->>Service: subscribers consume
sequenceDiagram
participant HTTP as HTTP Request
participant Handler as ServiceGlobalExceptionHandler
participant Base as BaseGlobalExceptionHandler
participant Response as HttpResponse
HTTP->>Handler: throws Exception
activate Handler
Handler->>Base: delegate to base handlers
activate Base
Base->>Base: errorCodePrefix() (override by service)
Base->>Response: return ApiError(code,status,message)
deactivate Base
deactivate Handler
Response-->>HTTP: 4xx/5xx + ApiError JSON
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
✨ Finishing Touches
🧪 Generate unit tests (beta)
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 26
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
fx-liquidity-engine/fx-liquidity-engine/src/test/java/com/stablecoin/payments/fx/infrastructure/scheduling/LockExpiryJobTest.java (1)
99-99:⚠️ Potential issue | 🟡 MinorFix semantically incorrect argument matcher in never() verification.
The
eqIgnoringTimestamps(anActiveLock(UUID.randomUUID()))matcher is unnecessary when verifying a method was never called. The random UUID also makes this matcher confusing since it won't match any actual invocation.🔧 Proposed fix
Replace with a simpler verification:
- then(lockService).should(never()).expireLock(eqIgnoringTimestamps(anActiveLock(UUID.randomUUID()))); + then(lockService).shouldHaveNoInteractions();Alternatively, if you specifically want to verify
expireLockwas not called:- then(lockService).should(never()).expireLock(eqIgnoringTimestamps(anActiveLock(UUID.randomUUID()))); + then(lockService).should(never()).expireLock(any());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fx-liquidity-engine/fx-liquidity-engine/src/test/java/com/stablecoin/payments/fx/infrastructure/scheduling/LockExpiryJobTest.java` at line 99, The verification is using a semantically incorrect matcher: remove the specific matcher eqIgnoringTimestamps(anActiveLock(UUID.randomUUID())) (and the meaningless UUID.randomUUID()) from the never() verification on lockService.expireLock; instead assert that expireLock was never invoked by using a generic any() matcher (e.g., expireLock(any(...))) or by verifying no interactions on lockService, referencing the lockService and expireLock symbols and removing eqIgnoringTimestamps and anActiveLock(UUID.randomUUID()) from this test assertion.api-gateway-iam/api-gateway-iam/src/integration-test/java/com/stablecoin/payments/gateway/iam/AbstractIntegrationTest.java (1)
29-59:⚠️ Potential issue | 🟠 MajorFlush Redis state between test methods.
REDISis now a static shared container, butcleanDatabase()still resets only Postgres. Cached API keys, revocation entries, or rate-limit counters can bleed across cases and make the integration suite order-dependent.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@api-gateway-iam/api-gateway-iam/src/integration-test/java/com/stablecoin/payments/gateway/iam/AbstractIntegrationTest.java` around lines 29 - 59, cleanDatabase() currently only truncates Postgres tables so Redis state can leak between tests; update cleanDatabase() to also flush Redis by invoking the test container REDIS (e.g. call REDIS.execInContainer("redis-cli", "FLUSHALL") or use a Redis client to connect to REDIS.getHost()/getMappedPort(...) and run FLUSHALL), and handle/propagate any exceptions so test setup fails loudly; reference the REDIS static container and the cleanDatabase() method when making the change.merchant-onboarding/merchant-onboarding/src/main/java/com/stablecoin/payments/merchant/onboarding/application/controller/GlobalExceptionHandler.java (1)
54-60:⚠️ Potential issue | 🟠 MajorScrub the generic error log in merchant onboarding.
log.error("Unexpected error: ", ex)records raw exception text and stack traces on the500path. In this service that can leak merchant/KYB data; log a stable error code and internal identifiers only.As per coding guidelines,
merchant-onboarding/**:KYB and merchant PII data must never appear in logs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@merchant-onboarding/merchant-onboarding/src/main/java/com/stablecoin/payments/merchant/onboarding/application/controller/GlobalExceptionHandler.java` around lines 54 - 60, The handleUnexpected method in GlobalExceptionHandler currently logs the raw exception and stacktrace (log.error("Unexpected error: ", ex)) which can leak PII; change it so it does NOT log the exception object or stacktrace. Instead generate a stable internal error identifier (e.g. UUID.randomUUID()) inside handleUnexpected, log only a concise message with the INTERNAL_ERROR_CODE and that identifier (e.g. log.error("INTERNAL_ERROR_CODE={} unexpected error id={}", INTERNAL_ERROR_CODE, errorId)), and return ApiError.of(INTERNAL_ERROR_CODE, INTERNAL_SERVER_ERROR.getReasonPhrase(), errorId) (or include the same identifier in the ApiError payload) so callers can reference the id; if you need the full exception for diagnostics, send ex to a secure error tracker separately (not to application logs). Ensure no usage of log.error(..., ex) or printing ex/stacktrace in handleUnexpected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@blockchain-custody/blockchain-custody/src/integration-test/java/com/stablecoin/payments/custody/AbstractIntegrationTest.java`:
- Around line 29-31: Tests share the static REDIS container so cached
wallet/nonce state bleeds between tests; add a Redis reset (e.g., execute
"FLUSHALL" via redis-cli inside the REDIS container) in the same test lifecycle
hook you use to reset PostgreSQL in AbstractIntegrationTest (the
`@BeforeEach/`@AfterEach method that resets POSTGRES) so Redis is cleared between
each test method; reference the static REDIS field and the existing lifecycle
method in AbstractIntegrationTest to implement the flush.
In
`@blockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/application/controller/GlobalExceptionHandler.java`:
- Around line 70-75: The handler handleInvalidState in GlobalExceptionHandler is
too broad because it catches java.lang.IllegalStateException; change its
`@ExceptionHandler` target and method signature to a custody-specific domain
exception (e.g., CustodyStateTransitionException or the existing domain
exception class that represents invalid custody transitions) so
framework/JPA/library IllegalStateExceptions are not mapped to BC-0002; keep the
`@ResponseStatus`(CONFLICT), the log call and the ApiError.of("BC-0002",
CONFLICT.getReasonPhrase(), ex.getMessage()) return, and update imports to
reference the domain exception class.
In `@build.gradle.kts`:
- Around line 5-8: The plugin declarations for org.springframework.boot,
io.spring.dependency-management, com.diffplug.spotless, and
com.google.cloud.tools.jib must be pinned to the centralized properties to
ensure reproducible builds: update the id(...) declarations to include the
corresponding version properties (use springBootVersion for
org.springframework.boot, springDependencyManagementVersion for
io.spring.dependency-management, spotlessVersion for com.diffplug.spotless, and
jibVersion for com.google.cloud.tools.jib) and add any missing property (e.g.,
springDependencyManagementVersion) to gradle.properties so all plugin versions
are explicit and consistent with the version catalog.
In `@buildSrc/build.gradle.kts`:
- Around line 10-15: The dependencies block currently hardcodes plugin versions
for org.springframework.boot:spring-boot-gradle-plugin,
io.spring.gradle:dependency-management-plugin,
com.diffplug.spotless:spotless-plugin-gradle, and
com.google.cloud.tools:jib-gradle-plugin; either document why these specific
versions were chosen by adding a brief comment above the dependencies block
mentioning sources/compatibility notes, or move these version strings into the
central Gradle version catalog (e.g., libs.versions.toml) and reference them
from buildSrc so versions are managed centrally; update the dependencies in the
dependencies { ... } block to use the catalog aliases (or leave them and add the
documentation comment) and ensure consistency with any existing version catalog
entries.
In `@buildSrc/src/main/kotlin/stablebridge.client-library.gradle.kts`:
- Around line 6-10: The project currently declares OpenFeign and test
dependencies directly in build scripts (e.g.,
implementation("org.springframework.cloud:spring-cloud-starter-openfeign") and
testImplementation/testRuntimeOnly entries); to centralize and prevent version
drift, add a Gradle version catalog (libs.versions.toml) that defines the Spring
Cloud OpenFeign and test artifact coordinates and update the client module build
scripts to reference the catalog aliases instead of hardcoded coordinates, or
alternatively add a short note in the repository README documenting that direct
coordinates are intentional and must be kept in sync; update references to the
dependency symbol
(implementation("org.springframework.cloud:spring-cloud-starter-openfeign")) and
the test dependency symbols to use the chosen centralization approach.
In `@buildSrc/src/main/kotlin/stablebridge.service.gradle.kts`:
- Around line 57-63: The shared test task registration (e.g.,
tasks.register<Test>("integrationTest") and the analogous task around lines
81-86) should not silently pass when no tests are discovered; set
failOnNoDiscoveredTests = true for these shared test tasks, and provide an
explicit per-module opt-out using a clear project property (for example check a
boolean project property like "allowEmptyIntegrationTests" or
"tests.allowEmpty") so modules that intentionally have empty suites can opt-out.
Update both the integrationTest registration and the other shared test task
registrations to enforce failOnNoDiscoveredTests = true by default and read the
opt-out property to override it when explicitly set.
- Around line 105-171: The dependencies block is adding a large unconditional
surface (Web, JPA, Security, Kafka, Feign, Resilience4j, OpenAPI, Outbox, Flyway
and the full integration-test set) to every project; split these into narrower,
opt-in convention plugins or feature toggles instead of keeping them all in the
single dependencies { ... } block. Create separate convention plugins (e.g.,
"convention-spring-web" for org.springframework.boot:spring-boot-starter-web and
springdoc-openapi, "convention-data-jpa" for spring-boot-starter-data-jpa,
Flyway and Postgres, "convention-security" for spring-boot-starter-security,
"convention-kafka" for spring-cloud-stream and spring-kafka, "convention-feign"
for spring-cloud-starter-openfeign, "convention-resilience4j" for resilience4j
libs, "convention-outbox" for io.namastack:namastack-outbox-starter-jdbc, and
"convention-integration-test" for the integrationTestImplementation entries) and
move the corresponding
implementation/runtimeOnly/test/integrationTestImplementation lines into those
plugins; then update the root convention to only apply truly universal deps (if
any) and document how services opt into the new plugins instead of inheriting
everything unconditionally.
In
`@compliance-travel-rule/compliance-travel-rule/src/main/java/com/stablecoin/payments/compliance/application/controller/GlobalExceptionHandler.java`:
- Around line 21-25: The service's GlobalExceptionHandler currently extends
BaseGlobalExceptionHandler which logs raw exception messages and stack traces
(risking PII); override the base logging methods in GlobalExceptionHandler
(e.g., any protected methods like logException, handleException, or
handleGlobalException used by BaseGlobalExceptionHandler) to suppress
exception.getMessage() and stack traces, emit only a sanitized log containing
the errorCode from errorCodePrefix(), non-PII identifiers (IDs not
names/addresses), request correlation IDs, and a generic error label, and ensure
HTTP responses do not contain PII; update GlobalExceptionHandler to perform this
sanitized logging and replace any calls that would log raw exceptions with the
new safe logging helpers.
In
`@compliance-travel-rule/compliance-travel-rule/src/main/java/com/stablecoin/payments/compliance/infrastructure/messaging/OutboxEventPublisher.java`:
- Around line 14-15: Replace the hardcoded partition key string in the
OutboxEventPublisher constructor with a class-level constant: declare a private
static final String PARTITION_KEY = "paymentId" and use PARTITION_KEY in the
constructor call (super(outbox, List.of(PARTITION_KEY))). Update references
inside the class to use PARTITION_KEY to avoid typos and ease refactors; ensure
visibility and immutability by keeping it private static final and name it in
all caps to follow convention.
In `@fiat-off-ramp/fiat-off-ramp/build.gradle.kts`:
- Around line 5-6: The build script reads resilience4jVersion and
wiremockVersion via project properties (resilience4jVersion, wiremockVersion in
build.gradle.kts); migrate these version entries into the version catalog
(libs.versions.toml) and replace uses in build.gradle.kts to reference the
catalog (e.g. libs.versions.<resilience4j|wiremock>.get() or the generated libs
accessor) instead of project.property(), removing the val resilience4jVersion /
wiremockVersion declarations; update any dependencyNotations that currently use
resilience4jVersion/wiremockVersion to use the catalog identifiers so
Dependabot/Renovate and central versioning are enabled.
In
`@fiat-off-ramp/fiat-off-ramp/src/main/java/com/stablecoin/payments/offramp/application/controller/GlobalExceptionHandler.java`:
- Around line 60-65: The handler handleInvalidState in GlobalExceptionHandler
currently catches java.lang.IllegalStateException and maps everything to
OF-0002; change it to catch a payout-specific exception (e.g.,
PayoutInvalidStateException or PayoutStateException) instead of
IllegalStateException by updating the `@ExceptionHandler` and method signature to
that custom exception and leaving the ApiError mapping to OF-0002; create/locate
and use the payout-specific exception where payout state transitions occur
(replace throws of IllegalStateException in payout logic with the new
PayoutInvalidStateException) so framework/container IllegalStateException
instances are no longer translated to OF-0002.
In `@fx-liquidity-engine/fx-liquidity-engine/build.gradle.kts`:
- Line 5: The project adds a service-specific resilience4j-retry dependency
(referenced via the resilience4jVersion property) that duplicates
circuit-breaker behavior from the platform plugin; either document why retry
semantics are required for FX rate fetching (and why Feign retry or Spring Retry
are insufficient) or remove the resilience4j-retry dependency from
build.gradle.kts to rely on the platform-provided resilience4j-circuitbreaker
and Feign/Spring retry facilities; update the dependency block or project README
with the rationale if you decide to keep it and ensure resilience4jVersion is
still used consistently.
In
`@merchant-iam/merchant-iam/src/test/java/com/stablecoin/payments/merchant/iam/arch/ArchitectureTest.java`:
- Around line 12-13: The test ArchitectureTest.verifyHexagonalArchitecture
currently calls
HexagonalArchitectureRules.verifyAll("com.stablecoin.payments.merchant.iam")
which applies the generic JPA/domain ban and will incorrectly flag allowed
`@Entity/`@Table/@Column uses; update the test to call a merchant-iam specific
verifier or an overload that disables the JPA prohibition (e.g.,
HexagonalArchitectureRules.verifyAllAllowJpa(...) or
HexagonalArchitectureRules.verifyAll(..., allowJpa=true)), or add a merchant-iam
specific method like
HexagonalArchitectureRules.verifyForMerchantIam("com.stablecoin.payments.merchant.iam")
so the verifier permits Spring JPA annotations in domain classes. Ensure the
change references the existing HexagonalArchitectureRules.verifyAll call in
ArchitectureTest.verifyHexagonalArchitecture and removes the hard-coded JPA ban
for this module.
In
`@merchant-onboarding/merchant-onboarding/src/test/java/com/stablecoin/payments/merchant/onboarding/arch/ArchitectureTest.java`:
- Around line 12-13: The test currently calls
HexagonalArchitectureRules.verifyAll("com.stablecoin.payments.merchant.onboarding")
which enforces the default domain-vs-JPA/Spring prohibitions and will flag
permitted annotations; update verifyHexagonalArchitecture() to invoke the
module-specific rule set or override that whitelists the allowed annotations
(e.g. configure HexagonalArchitectureRules to use an override/allowlist API or a
verifyWithOverrides/verifyCustomRules method) and include the Spring/domain
annotations `@Entity`, `@Table`, `@Column`, `@Transactional`, and `@Value` as permitted
for domain classes so the test no longer reports those as violations.
In `@payment-orchestrator/payment-orchestrator/build.gradle.kts`:
- Around line 23-24: Remove the explicit dependency
implementation("org.springframework.boot:spring-boot-starter-kafka") from the
build file because the convention plugin already provides spring-kafka and
spring-cloud-stream-binder-kafka; keep using your existing manual wiring (e.g.,
KafkaProducerConfig) and direct Spring Kafka APIs (`@KafkaListener`,
KafkaTemplate) without the redundant starter so the module relies on the
convention-provided Kafka artifacts instead of adding the starter.
In
`@payment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/application/controller/GlobalExceptionHandler.java`:
- Around line 28-32: Override the shared IllegalStateException handling in
GlobalExceptionHandler (which currently extends BaseGlobalExceptionHandler) by
adding an `@ExceptionHandler`(IllegalStateException.class) method (e.g.,
handleIllegalState or handlePaymentIllegalState) that does NOT let the base
handler swallow or genericize these errors; instead map or rethrow them as an
explicit domain exception (create and throw PaymentTransitionException or
PaymentStateTransitionException if one does not exist) so payment state
transition failures remain explicit and handled by domain-specific logic rather
than the shared BaseGlobalExceptionHandler path.
In `@platform-infra/build.gradle.kts`:
- Line 12: Replace the hardcoded dependency version for namastack by introducing
a property and referencing it: add namastackVersion=1.1.0 to gradle.properties,
then update the implementation statement that references
namastack-outbox-starter-jdbc (the
implementation("io.namastack:namastack-outbox-starter-jdbc:1.1.0") line) to use
the property via the $namastackVersion placeholder so the dependency becomes
implementation("io.namastack:namastack-outbox-starter-jdbc:$namastackVersion").
In
`@platform-infra/src/main/java/com/stablecoin/payments/platform/infrastructure/exception/BaseGlobalExceptionHandler.java`:
- Around line 32-35: The errors map built in BaseGlobalExceptionHandler uses
ex.getBindingResult().getFieldErrors() which drops bean-level/ObjectError
instances; change the aggregation to iterate over
ex.getBindingResult().getAllErrors(), and for each error use instanceof
FieldError to group by FieldError::getField and otherwise group by
ObjectError::getObjectName (or a fixed global key) while still mapping
ObjectError::getDefaultMessage so that class-level and cross-field validation
messages are included in the resulting errors map.
- Line 36: The handler currently logs the full validation map via
log.info("Validation failed: {}", errors) which can leak secrets; update
BaseGlobalExceptionHandler to sanitize or redact sensitive keys before logging:
in the method that builds or receives the errors map (referenced by the errors
variable in BaseGlobalExceptionHandler) create a sanitized copy that masks
values for keys like "password", "token", "clientSecret", "apiKey", "secret"
(and any case/underscore variants), then log the sanitized map or a safe summary
(e.g., field names with masked values or count of errors) instead of the raw
errors; apply the same sanitization approach to the other log sites flagged (the
similar log statements around the other occurrences).
- Around line 21-81: Add focused unit tests that instantiate a concrete subclass
of BaseGlobalExceptionHandler (override errorCodePrefix()) and directly exercise
handleValidation(MethodArgumentNotValidException),
handleConstraintViolation(ConstraintViolationException),
handleTypeMismatch(MethodArgumentTypeMismatchException),
handleIllegalArgument(IllegalArgumentException),
handleInvalidState(IllegalStateException), and handleUnexpected(Exception); for
each test construct the specific exception (mock a BindingResult/FieldError list
for MethodArgumentNotValidException and a ConstraintViolation set for
ConstraintViolationException), invoke the handler method, and assert the
returned ApiError contains the expected error code from
code("0001"/"0004"/"9999"), the correct HTTP reason phrase, the message or
errors map, and that logging paths are exercised (e.g., verify no NPEs and
validate grouped error keys/messages). Ensure the test class is concrete so the
abstract errorCodePrefix() is provided and covers both 400-series and 500
catch-all behavior.
In
`@platform-infra/src/main/java/com/stablecoin/payments/platform/infrastructure/messaging/AbstractOutboxEventPublisher.java`:
- Around line 25-36: The resolveKey method currently uses
String.valueOf(method.invoke(event)) which turns a null key into the literal
"null" string; change resolveKey to invoke the accessor (method.invoke(event)),
check the returned Object result for null and if null continue trying the next
fieldName in keyFieldNames instead of converting it to "null", and only convert
a non-null result to String before returning; if no non-null key is found after
iterating keyFieldNames, return null or throw a clear IllegalArgumentException
(choose the behavior consistent with callers) to avoid emitting the literal
"null" partition key.
- Line 28: The reflection in AbstractOutboxEventPublisher calling
event.getClass().getMethod(fieldName) assumes record-style accessors and will
fail for POJOs; update the logic to attempt the record accessor first and if
NoSuchMethodException is thrown, construct the JavaBean getter name by
capitalizing the first character of fieldName and prefixing with "get" (e.g.,
"getPaymentId") and try event.getClass().getMethod(getterName), or alternatively
document that events must be records—ensure the code references and handles
exceptions around event.getClass().getMethod(fieldName) and uses the fallback
getter lookup when the record accessor is absent.
In
`@platform-infra/src/main/java/com/stablecoin/payments/platform/infrastructure/messaging/AbstractOutboxHandler.java`:
- Around line 21-30: The base outbox handler logs potentially sensitive values
(the message key and exception message) in log.debug and log.error; update
AbstractOutboxHandler to avoid emitting metadata.getKey()/key and
e.getMessage(): change the log.debug call after kafkaTemplate.send(...).get(...)
to only include event.getClass().getSimpleName() and topic, and change the
log.error in the generic catch to log event type, topic and a stable error
classification such as e.getClass().getSimpleName() or a fixed string like
"publish-failure" (do not include the exception message or key); keep
InterruptedException handling but ensure Thread.currentThread().interrupt() is
still called and the thrown RuntimeException wraps the original exception
without exposing sensitive strings in logs.
- Around line 18-19: Introduce an OutboxEvent marker interface that exposes an
instance method String getTopic(), update all event classes to implement
OutboxEvent and have getTopic() return their existing TOPIC constant, and then
stop using reflection: in AbstractOutboxHandler replace
resolveStaticField(event, "TOPIC") with a cast to OutboxEvent and call
getTopic(); also update any method signatures or generics in
AbstractOutboxHandler that accept events to require OutboxEvent so
non-conforming events fail at compile time.
In
`@platform-test/src/testFixtures/java/com/stablecoin/payments/platform/test/HexagonalArchitectureRules.java`:
- Around line 17-30: The verifyAll method currently calls each rule's check()
sequentially which stops at the first failure; change it to build a single
composite rule (using CompositeArchRule.of(...)) combining
domainShouldNotDependOnSpring(), domainShouldNotDependOnJpa(),
domainShouldNotDependOnInfrastructure(), domainShouldNotDependOnApplication(),
infrastructureShouldNotDependOnController(), portsShouldBeInterfaces(),
domainEventsShouldBeRecords(), controllersShouldResideInApplicationController()
and call check(importedClasses) once; add the import for
com.tngtech.archunit.lang.CompositeArchRule and replace the multiple check()
invocations in verifyAll with the composite rule check to surface all violations
in one run.
In
`@platform-test/src/testFixtures/java/com/stablecoin/payments/platform/test/TestContainerSupport.java`:
- Around line 10-79: Add unit tests for the new public helpers: verify
postgres(String) returns a PostgreSQLContainer configured with image
"postgres:16-alpine" and the provided databaseName/username/password (test
PostgreSQLContainer<?> postgres(String)), verify kafka() uses
"confluentinc/cp-kafka:7.6.0" (test KafkaContainer kafka()), and verify redis()
exposes port 6379 (test GenericContainer<?> redis()). Add tests for
registerPostgresProperties, registerKafkaProperties and registerRedisProperties
by passing a mock or test DynamicPropertyRegistry and asserting the expected
keys map to the container getters (postgres::getJdbcUrl,
kafka::getBootstrapServers, redis::getHost/getMappedPort). Add a startAll
cleanup test that supplies multiple Startable mocks where one throws on start
and assert safeStop was called on the others and the original RuntimeException
is propagated (use Mockito spies/mocks to verify stop() calls); also add a
safeStop test that ensures exceptions from stop() are swallowed (mock a
Startable that throws on stop and assert no exception escapes).
---
Outside diff comments:
In
`@api-gateway-iam/api-gateway-iam/src/integration-test/java/com/stablecoin/payments/gateway/iam/AbstractIntegrationTest.java`:
- Around line 29-59: cleanDatabase() currently only truncates Postgres tables so
Redis state can leak between tests; update cleanDatabase() to also flush Redis
by invoking the test container REDIS (e.g. call
REDIS.execInContainer("redis-cli", "FLUSHALL") or use a Redis client to connect
to REDIS.getHost()/getMappedPort(...) and run FLUSHALL), and handle/propagate
any exceptions so test setup fails loudly; reference the REDIS static container
and the cleanDatabase() method when making the change.
In
`@fx-liquidity-engine/fx-liquidity-engine/src/test/java/com/stablecoin/payments/fx/infrastructure/scheduling/LockExpiryJobTest.java`:
- Line 99: The verification is using a semantically incorrect matcher: remove
the specific matcher eqIgnoringTimestamps(anActiveLock(UUID.randomUUID())) (and
the meaningless UUID.randomUUID()) from the never() verification on
lockService.expireLock; instead assert that expireLock was never invoked by
using a generic any() matcher (e.g., expireLock(any(...))) or by verifying no
interactions on lockService, referencing the lockService and expireLock symbols
and removing eqIgnoringTimestamps and anActiveLock(UUID.randomUUID()) from this
test assertion.
In
`@merchant-onboarding/merchant-onboarding/src/main/java/com/stablecoin/payments/merchant/onboarding/application/controller/GlobalExceptionHandler.java`:
- Around line 54-60: The handleUnexpected method in GlobalExceptionHandler
currently logs the raw exception and stacktrace (log.error("Unexpected error: ",
ex)) which can leak PII; change it so it does NOT log the exception object or
stacktrace. Instead generate a stable internal error identifier (e.g.
UUID.randomUUID()) inside handleUnexpected, log only a concise message with the
INTERNAL_ERROR_CODE and that identifier (e.g. log.error("INTERNAL_ERROR_CODE={}
unexpected error id={}", INTERNAL_ERROR_CODE, errorId)), and return
ApiError.of(INTERNAL_ERROR_CODE, INTERNAL_SERVER_ERROR.getReasonPhrase(),
errorId) (or include the same identifier in the ApiError payload) so callers can
reference the id; if you need the full exception for diagnostics, send ex to a
secure error tracker separately (not to application logs). Ensure no usage of
log.error(..., ex) or printing ex/stacktrace in handleUnexpected.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d5aa72de-f118-485d-bf67-7bc60eb02908
📒 Files selected for processing (146)
api-gateway-iam/api-gateway-iam-api/build.gradle.ktsapi-gateway-iam/api-gateway-iam-api/src/main/java/com/stablecoin/payments/gateway/iam/api/response/ApiError.javaapi-gateway-iam/api-gateway-iam-client/build.gradle.ktsapi-gateway-iam/api-gateway-iam/build.gradle.ktsapi-gateway-iam/api-gateway-iam/src/integration-test/java/com/stablecoin/payments/gateway/iam/AbstractIntegrationTest.javaapi-gateway-iam/api-gateway-iam/src/integration-test/java/com/stablecoin/payments/gateway/iam/config/TestSecurityConfig.javaapi-gateway-iam/api-gateway-iam/src/main/java/com/stablecoin/payments/gateway/iam/application/controller/GlobalExceptionHandler.javaapi-gateway-iam/api-gateway-iam/src/main/java/com/stablecoin/payments/gateway/iam/infrastructure/messaging/GatewayIamOutboxHandler.javaapi-gateway-iam/api-gateway-iam/src/main/java/com/stablecoin/payments/gateway/iam/infrastructure/messaging/OutboxEventPublisher.javaapi-gateway-iam/api-gateway-iam/src/test/java/com/stablecoin/payments/gateway/iam/ArchitectureTest.javaapi-gateway-iam/api-gateway-iam/src/test/java/com/stablecoin/payments/gateway/iam/arch/ArchitectureTest.javaapi-gateway-iam/api-gateway-iam/src/test/java/com/stablecoin/payments/gateway/iam/domain/service/ApiKeyCommandHandlerTest.javaapi-gateway-iam/api-gateway-iam/src/test/java/com/stablecoin/payments/gateway/iam/domain/service/AuthCommandHandlerTest.javaapi-gateway-iam/api-gateway-iam/src/test/java/com/stablecoin/payments/gateway/iam/domain/service/MerchantCommandHandlerTest.javaapi-gateway-iam/api-gateway-iam/src/test/java/com/stablecoin/payments/gateway/iam/domain/service/OAuthClientCommandHandlerTest.javaapi-gateway-iam/api-gateway-iam/src/testFixtures/java/com/stablecoin/payments/gateway/iam/fixtures/TestUtils.javablockchain-custody/blockchain-custody-api/build.gradle.ktsblockchain-custody/blockchain-custody-api/src/main/java/com/stablecoin/payments/custody/api/ApiError.javablockchain-custody/blockchain-custody-client/build.gradle.ktsblockchain-custody/blockchain-custody/build.gradle.ktsblockchain-custody/blockchain-custody/src/integration-test/java/com/stablecoin/payments/custody/AbstractIntegrationTest.javablockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/application/controller/GlobalExceptionHandler.javablockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/infrastructure/messaging/CustodyOutboxEventPublisher.javablockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/infrastructure/messaging/CustodyOutboxHandler.javablockchain-custody/blockchain-custody/src/test/java/com/stablecoin/payments/custody/arch/ArchitectureTest.javablockchain-custody/blockchain-custody/src/test/java/com/stablecoin/payments/custody/domain/service/BalanceSyncCommandHandlerTest.javablockchain-custody/blockchain-custody/src/test/java/com/stablecoin/payments/custody/domain/service/TransferCommandHandlerTest.javablockchain-custody/blockchain-custody/src/test/java/com/stablecoin/payments/custody/domain/service/TransferMonitorCommandHandlerTest.javabuild.gradle.ktsbuildSrc/build.gradle.ktsbuildSrc/settings.gradle.ktsbuildSrc/src/main/kotlin/stablebridge.api-library.gradle.ktsbuildSrc/src/main/kotlin/stablebridge.client-library.gradle.ktsbuildSrc/src/main/kotlin/stablebridge.service.gradle.ktscompliance-travel-rule/compliance-travel-rule-api/build.gradle.ktscompliance-travel-rule/compliance-travel-rule-api/src/main/java/com/stablecoin/payments/compliance/api/response/ApiError.javacompliance-travel-rule/compliance-travel-rule-client/build.gradle.ktscompliance-travel-rule/compliance-travel-rule/build.gradle.ktscompliance-travel-rule/compliance-travel-rule/src/integration-test/java/com/stablecoin/payments/compliance/AbstractIntegrationTest.javacompliance-travel-rule/compliance-travel-rule/src/integration-test/java/com/stablecoin/payments/compliance/config/TestSecurityConfig.javacompliance-travel-rule/compliance-travel-rule/src/main/java/com/stablecoin/payments/compliance/application/controller/GlobalExceptionHandler.javacompliance-travel-rule/compliance-travel-rule/src/main/java/com/stablecoin/payments/compliance/infrastructure/messaging/ComplianceOutboxHandler.javacompliance-travel-rule/compliance-travel-rule/src/main/java/com/stablecoin/payments/compliance/infrastructure/messaging/OutboxEventPublisher.javacompliance-travel-rule/compliance-travel-rule/src/test/java/com/stablecoin/payments/compliance/arch/ArchitectureTest.javacompliance-travel-rule/compliance-travel-rule/src/testFixtures/java/com/stablecoin/payments/compliance/fixtures/TestUtils.javafiat-off-ramp/fiat-off-ramp-api/build.gradle.ktsfiat-off-ramp/fiat-off-ramp-api/src/main/java/com/stablecoin/payments/offramp/api/ApiError.javafiat-off-ramp/fiat-off-ramp-client/build.gradle.ktsfiat-off-ramp/fiat-off-ramp/build.gradle.ktsfiat-off-ramp/fiat-off-ramp/src/integration-test/java/com/stablecoin/payments/offramp/AbstractIntegrationTest.javafiat-off-ramp/fiat-off-ramp/src/main/java/com/stablecoin/payments/offramp/application/controller/GlobalExceptionHandler.javafiat-off-ramp/fiat-off-ramp/src/main/java/com/stablecoin/payments/offramp/infrastructure/messaging/OffRampOutboxEventPublisher.javafiat-off-ramp/fiat-off-ramp/src/main/java/com/stablecoin/payments/offramp/infrastructure/messaging/OffRampOutboxHandler.javafiat-off-ramp/fiat-off-ramp/src/test/java/com/stablecoin/payments/offramp/arch/ArchitectureTest.javafiat-off-ramp/fiat-off-ramp/src/test/java/com/stablecoin/payments/offramp/domain/service/PayoutCommandHandlerTest.javafiat-off-ramp/fiat-off-ramp/src/test/java/com/stablecoin/payments/offramp/domain/service/PayoutMonitorCommandHandlerTest.javafiat-off-ramp/fiat-off-ramp/src/test/java/com/stablecoin/payments/offramp/domain/service/WebhookCommandHandlerTest.javafiat-off-ramp/fiat-off-ramp/src/testFixtures/java/com/stablecoin/payments/offramp/fixtures/TestUtils.javafiat-on-ramp/fiat-on-ramp-api/build.gradle.ktsfiat-on-ramp/fiat-on-ramp-client/build.gradle.ktsfiat-on-ramp/fiat-on-ramp/build.gradle.ktsfiat-on-ramp/fiat-on-ramp/src/integration-test/java/com/stablecoin/payments/onramp/AbstractIntegrationTest.javafiat-on-ramp/fiat-on-ramp/src/main/java/com/stablecoin/payments/onramp/application/controller/GlobalExceptionHandler.javafiat-on-ramp/fiat-on-ramp/src/main/java/com/stablecoin/payments/onramp/infrastructure/messaging/OnRampOutboxEventPublisher.javafiat-on-ramp/fiat-on-ramp/src/main/java/com/stablecoin/payments/onramp/infrastructure/messaging/OnRampOutboxHandler.javafiat-on-ramp/fiat-on-ramp/src/test/java/com/stablecoin/payments/onramp/application/controller/StripeWebhookControllerTest.javafiat-on-ramp/fiat-on-ramp/src/test/java/com/stablecoin/payments/onramp/arch/ArchitectureTest.javafiat-on-ramp/fiat-on-ramp/src/test/java/com/stablecoin/payments/onramp/domain/service/CollectionCommandHandlerTest.javafiat-on-ramp/fiat-on-ramp/src/test/java/com/stablecoin/payments/onramp/domain/service/ReconciliationCommandHandlerTest.javafiat-on-ramp/fiat-on-ramp/src/test/java/com/stablecoin/payments/onramp/domain/service/RefundCommandHandlerTest.javafiat-on-ramp/fiat-on-ramp/src/test/java/com/stablecoin/payments/onramp/domain/service/WebhookCommandHandlerTest.javafiat-on-ramp/fiat-on-ramp/src/testFixtures/java/com/stablecoin/payments/onramp/fixtures/TestUtils.javafx-liquidity-engine/fx-liquidity-engine-api/build.gradle.ktsfx-liquidity-engine/fx-liquidity-engine-api/src/main/java/com/stablecoin/payments/fx/api/response/ApiError.javafx-liquidity-engine/fx-liquidity-engine-client/build.gradle.ktsfx-liquidity-engine/fx-liquidity-engine/build.gradle.ktsfx-liquidity-engine/fx-liquidity-engine/src/integration-test/java/com/stablecoin/payments/fx/AbstractIntegrationTest.javafx-liquidity-engine/fx-liquidity-engine/src/main/java/com/stablecoin/payments/fx/application/controller/GlobalExceptionHandler.javafx-liquidity-engine/fx-liquidity-engine/src/main/java/com/stablecoin/payments/fx/infrastructure/messaging/FxOutboxHandler.javafx-liquidity-engine/fx-liquidity-engine/src/main/java/com/stablecoin/payments/fx/infrastructure/messaging/OutboxEventPublisher.javafx-liquidity-engine/fx-liquidity-engine/src/test/java/com/stablecoin/payments/fx/ArchitectureTest.javafx-liquidity-engine/fx-liquidity-engine/src/test/java/com/stablecoin/payments/fx/application/controller/GlobalExceptionHandlerTest.javafx-liquidity-engine/fx-liquidity-engine/src/test/java/com/stablecoin/payments/fx/application/service/FxQuoteApplicationServiceTest.javafx-liquidity-engine/fx-liquidity-engine/src/test/java/com/stablecoin/payments/fx/arch/ArchitectureTest.javafx-liquidity-engine/fx-liquidity-engine/src/test/java/com/stablecoin/payments/fx/infrastructure/scheduling/LockExpiryJobTest.javafx-liquidity-engine/fx-liquidity-engine/src/test/java/com/stablecoin/payments/fx/infrastructure/scheduling/RateRefreshJobTest.javafx-liquidity-engine/fx-liquidity-engine/src/testFixtures/java/com/stablecoin/payments/fx/fixtures/TestUtils.javaledger-accounting/ledger-accounting-api/build.gradle.ktsledger-accounting/ledger-accounting-api/src/main/java/com/stablecoin/payments/ledger/api/ApiError.javaledger-accounting/ledger-accounting-client/build.gradle.ktsledger-accounting/ledger-accounting/build.gradle.ktsledger-accounting/ledger-accounting/src/integration-test/java/com/stablecoin/payments/ledger/AbstractIntegrationTest.javaledger-accounting/ledger-accounting/src/main/java/com/stablecoin/payments/ledger/application/controller/GlobalExceptionHandler.javaledger-accounting/ledger-accounting/src/main/java/com/stablecoin/payments/ledger/infrastructure/messaging/LedgerOutboxHandler.javaledger-accounting/ledger-accounting/src/test/java/com/stablecoin/payments/ledger/arch/ArchitectureTest.javaledger-accounting/ledger-accounting/src/test/java/com/stablecoin/payments/ledger/domain/service/JournalCommandHandlerTest.javaledger-accounting/ledger-accounting/src/test/java/com/stablecoin/payments/ledger/domain/service/ReconciliationCommandHandlerTest.javaledger-accounting/ledger-accounting/src/testFixtures/java/com/stablecoin/payments/ledger/fixtures/TestUtils.javamerchant-iam/merchant-iam-api/build.gradle.ktsmerchant-iam/merchant-iam-client/build.gradle.ktsmerchant-iam/merchant-iam/build.gradle.ktsmerchant-iam/merchant-iam/src/integration-test/java/com/stablecoin/payments/merchant/iam/AbstractIntegrationTest.javamerchant-iam/merchant-iam/src/main/java/com/stablecoin/payments/merchant/iam/application/controller/ApiError.javamerchant-iam/merchant-iam/src/main/java/com/stablecoin/payments/merchant/iam/application/controller/GlobalExceptionHandler.javamerchant-iam/merchant-iam/src/main/java/com/stablecoin/payments/merchant/iam/infrastructure/messaging/MerchantIamOutboxHandler.javamerchant-iam/merchant-iam/src/main/java/com/stablecoin/payments/merchant/iam/infrastructure/messaging/OutboxEventPublisher.javamerchant-iam/merchant-iam/src/test/java/com/stablecoin/payments/merchant/iam/arch/ArchitectureTest.javamerchant-iam/merchant-iam/src/test/java/com/stablecoin/payments/merchant/iam/domain/team/AuthServiceTest.javamerchant-iam/merchant-iam/src/test/java/com/stablecoin/payments/merchant/iam/domain/team/MerchantTeamServiceTest.javamerchant-iam/merchant-iam/src/testFixtures/java/com/stablecoin/payments/merchant/iam/fixtures/TestUtils.javamerchant-onboarding/merchant-onboarding-api/build.gradle.ktsmerchant-onboarding/merchant-onboarding-client/build.gradle.ktsmerchant-onboarding/merchant-onboarding/build.gradle.ktsmerchant-onboarding/merchant-onboarding/src/integration-test/java/com/stablecoin/payments/merchant/onboarding/AbstractIntegrationTest.javamerchant-onboarding/merchant-onboarding/src/integration-test/java/com/stablecoin/payments/merchant/onboarding/config/TestSecurityConfig.javamerchant-onboarding/merchant-onboarding/src/main/java/com/stablecoin/payments/merchant/onboarding/application/controller/ApiError.javamerchant-onboarding/merchant-onboarding/src/main/java/com/stablecoin/payments/merchant/onboarding/application/controller/GlobalExceptionHandler.javamerchant-onboarding/merchant-onboarding/src/main/java/com/stablecoin/payments/merchant/onboarding/infrastructure/messaging/OnboardingOutboxHandler.javamerchant-onboarding/merchant-onboarding/src/test/java/com/stablecoin/payments/merchant/onboarding/arch/ArchitectureTest.javamerchant-onboarding/merchant-onboarding/src/test/java/com/stablecoin/payments/merchant/onboarding/domain/merchant/MerchantCommandHandlerTest.javamerchant-onboarding/merchant-onboarding/src/testFixtures/java/com/stablecoin/payments/merchant/onboarding/fixtures/TestUtils.javapayment-orchestrator/payment-orchestrator-api/build.gradle.ktspayment-orchestrator/payment-orchestrator-api/src/main/java/com/stablecoin/payments/orchestrator/api/ApiError.javapayment-orchestrator/payment-orchestrator-client/build.gradle.ktspayment-orchestrator/payment-orchestrator/build.gradle.ktspayment-orchestrator/payment-orchestrator/src/integration-test/java/com/stablecoin/payments/orchestrator/AbstractIntegrationTest.javapayment-orchestrator/payment-orchestrator/src/integration-test/resources/application-integration-test.ymlpayment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/application/controller/GlobalExceptionHandler.javapayment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/infrastructure/messaging/OrchestratorOutboxHandler.javapayment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/infrastructure/messaging/OutboxEventPublisher.javapayment-orchestrator/payment-orchestrator/src/test/java/com/stablecoin/payments/orchestrator/arch/ArchitectureTest.javapayment-orchestrator/payment-orchestrator/src/test/java/com/stablecoin/payments/orchestrator/domain/service/PaymentCommandHandlerTest.javapayment-orchestrator/payment-orchestrator/src/test/java/com/stablecoin/payments/orchestrator/infrastructure/activity/EventPublishingActivityImplTest.javapayment-orchestrator/payment-orchestrator/src/testFixtures/java/com/stablecoin/payments/orchestrator/fixtures/TestUtils.javaplatform-api/build.gradle.ktsplatform-api/src/main/java/com/stablecoin/payments/platform/api/ApiError.javaplatform-infra/build.gradle.ktsplatform-infra/src/main/java/com/stablecoin/payments/platform/infrastructure/exception/BaseGlobalExceptionHandler.javaplatform-infra/src/main/java/com/stablecoin/payments/platform/infrastructure/messaging/AbstractOutboxEventPublisher.javaplatform-infra/src/main/java/com/stablecoin/payments/platform/infrastructure/messaging/AbstractOutboxHandler.javaplatform-test/build.gradle.ktsplatform-test/src/testFixtures/java/com/stablecoin/payments/platform/test/HexagonalArchitectureRules.javaplatform-test/src/testFixtures/java/com/stablecoin/payments/platform/test/TestContainerSupport.javaplatform-test/src/testFixtures/java/com/stablecoin/payments/platform/test/TestSecurityConfig.javaplatform-test/src/testFixtures/java/com/stablecoin/payments/platform/test/TestUtils.javasettings.gradle.kts
💤 Files with no reviewable changes (23)
- api-gateway-iam/api-gateway-iam/src/testFixtures/java/com/stablecoin/payments/gateway/iam/fixtures/TestUtils.java
- compliance-travel-rule/compliance-travel-rule/src/integration-test/java/com/stablecoin/payments/compliance/config/TestSecurityConfig.java
- api-gateway-iam/api-gateway-iam/src/test/java/com/stablecoin/payments/gateway/iam/ArchitectureTest.java
- fx-liquidity-engine/fx-liquidity-engine-api/src/main/java/com/stablecoin/payments/fx/api/response/ApiError.java
- compliance-travel-rule/compliance-travel-rule-api/src/main/java/com/stablecoin/payments/compliance/api/response/ApiError.java
- blockchain-custody/blockchain-custody-api/src/main/java/com/stablecoin/payments/custody/api/ApiError.java
- api-gateway-iam/api-gateway-iam-api/src/main/java/com/stablecoin/payments/gateway/iam/api/response/ApiError.java
- merchant-iam/merchant-iam/src/main/java/com/stablecoin/payments/merchant/iam/application/controller/ApiError.java
- fiat-off-ramp/fiat-off-ramp-api/src/main/java/com/stablecoin/payments/offramp/api/ApiError.java
- fiat-on-ramp/fiat-on-ramp/src/testFixtures/java/com/stablecoin/payments/onramp/fixtures/TestUtils.java
- merchant-onboarding/merchant-onboarding/src/integration-test/java/com/stablecoin/payments/merchant/onboarding/config/TestSecurityConfig.java
- merchant-onboarding/merchant-onboarding/src/main/java/com/stablecoin/payments/merchant/onboarding/application/controller/ApiError.java
- merchant-iam/merchant-iam/src/testFixtures/java/com/stablecoin/payments/merchant/iam/fixtures/TestUtils.java
- merchant-onboarding/merchant-onboarding/src/testFixtures/java/com/stablecoin/payments/merchant/onboarding/fixtures/TestUtils.java
- fx-liquidity-engine/fx-liquidity-engine/src/test/java/com/stablecoin/payments/fx/ArchitectureTest.java
- fx-liquidity-engine/fx-liquidity-engine/src/testFixtures/java/com/stablecoin/payments/fx/fixtures/TestUtils.java
- payment-orchestrator/payment-orchestrator-api/src/main/java/com/stablecoin/payments/orchestrator/api/ApiError.java
- api-gateway-iam/api-gateway-iam/src/integration-test/java/com/stablecoin/payments/gateway/iam/config/TestSecurityConfig.java
- payment-orchestrator/payment-orchestrator/src/testFixtures/java/com/stablecoin/payments/orchestrator/fixtures/TestUtils.java
- fiat-off-ramp/fiat-off-ramp/src/testFixtures/java/com/stablecoin/payments/offramp/fixtures/TestUtils.java
- ledger-accounting/ledger-accounting/src/testFixtures/java/com/stablecoin/payments/ledger/fixtures/TestUtils.java
- ledger-accounting/ledger-accounting-api/src/main/java/com/stablecoin/payments/ledger/api/ApiError.java
- compliance-travel-rule/compliance-travel-rule/src/testFixtures/java/com/stablecoin/payments/compliance/fixtures/TestUtils.java
Comment on lines
+70
to
75
| @Override | ||
| @ResponseStatus(CONFLICT) | ||
| @ExceptionHandler(IllegalStateException.class) | ||
| public ApiError handleIllegalState(IllegalStateException ex) { | ||
| public ApiError handleInvalidState(IllegalStateException ex) { | ||
| log.info("Invalid state transition: {}", ex.getMessage()); | ||
| return ApiError.of("BC-0002", CONFLICT.getReasonPhrase(), ex.getMessage()); |
There was a problem hiding this comment.
Narrow this conflict mapping to a domain exception.
Overriding the shared handler on plain IllegalStateException means framework/JPA/library state failures also come back as BC-0002 conflicts with their internal message. Bind this advice to a custody-specific transition exception instead.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@blockchain-custody/blockchain-custody/src/main/java/com/stablecoin/payments/custody/application/controller/GlobalExceptionHandler.java`
around lines 70 - 75, The handler handleInvalidState in GlobalExceptionHandler
is too broad because it catches java.lang.IllegalStateException; change its
`@ExceptionHandler` target and method signature to a custody-specific domain
exception (e.g., CustodyStateTransitionException or the existing domain
exception class that represents invalid custody transitions) so
framework/JPA/library IllegalStateExceptions are not mapped to BC-0002; keep the
`@ResponseStatus`(CONFLICT), the log call and the ApiError.of("BC-0002",
CONFLICT.getReasonPhrase(), ex.getMessage()) return, and update imports to
reference the domain exception class.
Comment on lines
+5
to
+8
| id("org.springframework.boot") apply false | ||
| id("io.spring.dependency-management") apply false | ||
| id("com.diffplug.spotless") apply false | ||
| id("com.google.cloud.tools.jib") apply false |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== settings.gradle(.kts) files =="
fd 'settings\.gradle(\.kts)?$' -t f
SETTINGS_FILES="$(fd 'settings\.gradle(\.kts)?$' -t f | tr '\n' ' ')"
if [ -n "${SETTINGS_FILES// }" ]; then
echo "== pluginManagement blocks =="
rg -n --type=gradle 'pluginManagement|plugins\s*\{' $SETTINGS_FILES
fi
echo "== root plugin declarations impacted =="
rg -n --type=gradle 'id\("org\.springframework\.boot"\)|id\("io\.spring\.dependency-management"\)|id\("com\.diffplug\.spotless"\)|id\("com\.google\.cloud\.tools\.jib"\)' build.gradle.kts
echo "== version catalog presence and entries =="
CATALOG_FILES="$(fd 'libs\.versions\.toml$' -t f | tr '\n' ' ')"
if [ -n "${CATALOG_FILES// }" ]; then
rg -n 'spring|boot|dependency|spotless|jib' $CATALOG_FILES
else
echo "No libs.versions.toml found"
fiRepository: Puneethkumarck/stablebridge-platform
Length of output: 192
🏁 Script executed:
cat -n settings.gradle.ktsRepository: Puneethkumarck/stablebridge-platform
Length of output: 2262
🏁 Script executed:
cat -n buildSrc/settings.gradle.ktsRepository: Puneethkumarck/stablebridge-platform
Length of output: 172
🏁 Script executed:
fd 'libs\.versions\.toml$' -t fRepository: Puneethkumarck/stablebridge-platform
Length of output: 62
🏁 Script executed:
rg -A 10 'pluginManagement' settings.gradle.kts buildSrc/settings.gradle.kts 2>/dev/null || echo "No pluginManagement found"Repository: Puneethkumarck/stablebridge-platform
Length of output: 105
🏁 Script executed:
head -n 15 build.gradle.ktsRepository: Puneethkumarck/stablebridge-platform
Length of output: 505
🏁 Script executed:
cat gradle.properties 2>/dev/null || echo "No gradle.properties found"Repository: Puneethkumarck/stablebridge-platform
Length of output: 548
🏁 Script executed:
git log --oneline -n 5 -- build.gradle.kts 2>/dev/null || echo "Git history not available"Repository: Puneethkumarck/stablebridge-platform
Length of output: 168
Pin plugin versions from gradle.properties to ensure reproducible builds.
Lines 5-8 declare plugins without versions while springBootVersion, spotlessVersion, and jibVersion are already centralized in gradle.properties. Without explicit version pins or a pluginManagement block, Gradle resolves these plugins non-deterministically from the plugin portal. This conflicts with the coding guideline requiring version catalog alignment and creates build reproducibility risk.
Apply versions to the plugin declarations:
id("org.springframework.boot") version springBootVersion apply false
id("io.spring.dependency-management") version springDependencyManagementVersion apply false
id("com.diffplug.spotless") version spotlessVersion apply false
id("com.google.cloud.tools.jib") version jibVersion apply falseAdd any missing version properties (e.g., springDependencyManagementVersion) to gradle.properties to match your other plugins. Note that sonarqube on line 9 already has an explicit version—extend this consistency to all plugins.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@build.gradle.kts` around lines 5 - 8, The plugin declarations for
org.springframework.boot, io.spring.dependency-management,
com.diffplug.spotless, and com.google.cloud.tools.jib must be pinned to the
centralized properties to ensure reproducible builds: update the id(...)
declarations to include the corresponding version properties (use
springBootVersion for org.springframework.boot,
springDependencyManagementVersion for io.spring.dependency-management,
spotlessVersion for com.diffplug.spotless, and jibVersion for
com.google.cloud.tools.jib) and add any missing property (e.g.,
springDependencyManagementVersion) to gradle.properties so all plugin versions
are explicit and consistent with the version catalog.
Comment on lines
+10
to
+15
| dependencies { | ||
| implementation("org.springframework.boot:spring-boot-gradle-plugin:4.0.3") | ||
| implementation("io.spring.gradle:dependency-management-plugin:1.1.7") | ||
| implementation("com.diffplug.spotless:spotless-plugin-gradle:8.3.0") | ||
| implementation("com.google.cloud.tools:jib-gradle-plugin:3.5.3") | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Newly added buildSrc dependencies — confirm versions.
Flagging new dependencies per guidelines:
spring-boot-gradle-plugin:4.0.3dependency-management-plugin:1.1.7spotless-plugin-gradle:8.3.0jib-gradle-plugin:3.5.3
Hardcoded versions in buildSrc are acceptable, but consider documenting version choices or linking to a central version catalog for future maintenance.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@buildSrc/build.gradle.kts` around lines 10 - 15, The dependencies block
currently hardcodes plugin versions for
org.springframework.boot:spring-boot-gradle-plugin,
io.spring.gradle:dependency-management-plugin,
com.diffplug.spotless:spotless-plugin-gradle, and
com.google.cloud.tools:jib-gradle-plugin; either document why these specific
versions were chosen by adding a brief comment above the dependencies block
mentioning sources/compatibility notes, or move these version strings into the
central Gradle version catalog (e.g., libs.versions.toml) and reference them
from buildSrc so versions are managed centrally; update the dependencies in the
dependencies { ... } block to use the catalog aliases (or leave them and add the
documentation comment) and ensure consistency with any existing version catalog
entries.
Comment on lines
+6
to
+10
| implementation("org.springframework.cloud:spring-cloud-starter-openfeign") | ||
|
|
||
| testImplementation("org.junit.jupiter:junit-jupiter") | ||
| testImplementation("org.assertj:assertj-core") | ||
| testRuntimeOnly("org.junit.platform:junit-platform-launcher") |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Version catalog files =="
fd 'libs\.versions\.toml$'
echo
echo "== Direct coordinates in client-library convention =="
rg -n 'spring-cloud-starter-openfeign|org\.junit\.jupiter:junit-jupiter|org\.assertj:assertj-core|org\.junit\.platform:junit-platform-launcher' buildSrc/src/main/kotlin/stablebridge.client-library.gradle.kts
echo
echo "== Matching aliases/entries in version catalog (if present) =="
fd 'libs\.versions\.toml$' -x rg -n 'openfeign|junit|assertj|launcher' {}
echo
echo "== Modules applying stablebridge.client-library =="
rg -n --type-add 'gradle:*.gradle.kts' --type gradle 'stablebridge\.client-library'Repository: Puneethkumarck/stablebridge-platform
Length of output: 1535
🏁 Script executed:
# Check the convention file structure and history context
cat -n buildSrc/src/main/kotlin/stablebridge.client-library.gradle.kts
echo "=== Check if any modules override/exclude the convention ==="
rg -n 'openfeign' --type gradle -i
echo "=== Check if OpenFeign is actually configured/used ==="
rg -n '@EnableFeignClients|@FeignClient|FeignClientBuilder' --type java --type kotlin -iRepository: Puneethkumarck/stablebridge-platform
Length of output: 4740
OpenFeign usage confirmed across all client modules; consider adopting version catalog for dependency centralization.
OpenFeign is actively used via @FeignClient and @EnableFeignClients annotations across all 10 client modules, so the dependency is necessary. Test dependencies are correctly scoped with no main-compile leakage.
The codebase currently lacks a version catalog (libs.versions.toml), so hardcoded coordinates are the existing pattern. To prevent dependency drift across modules, evaluate adopting Gradle's version catalog or explicitly document that direct coordinates are intentional. This keeps dependency versions synchronized without manual updates to each module's build script.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@buildSrc/src/main/kotlin/stablebridge.client-library.gradle.kts` around lines
6 - 10, The project currently declares OpenFeign and test dependencies directly
in build scripts (e.g.,
implementation("org.springframework.cloud:spring-cloud-starter-openfeign") and
testImplementation/testRuntimeOnly entries); to centralize and prevent version
drift, add a Gradle version catalog (libs.versions.toml) that defines the Spring
Cloud OpenFeign and test artifact coordinates and update the client module build
scripts to reference the catalog aliases instead of hardcoded coordinates, or
alternatively add a short note in the repository README documenting that direct
coordinates are intentional and must be kept in sync; update references to the
dependency symbol
(implementation("org.springframework.cloud:spring-cloud-starter-openfeign")) and
the test dependency symbols to use the chosen centralization approach.
| private String resolveKey(Object event) { | ||
| for (String fieldName : keyFieldNames) { | ||
| try { | ||
| var method = event.getClass().getMethod(fieldName); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if event classes are records or have matching accessor patterns
echo "=== Searching for event class definitions ==="
rg -nP "class \w+Event\b" --type java -A 3 | head -60
echo ""
echo "=== Checking for record event definitions ==="
rg -nP "record \w+Event\b" --type java | head -30Repository: Puneethkumarck/stablebridge-platform
Length of output: 7083
🏁 Script executed:
# Get context around the problematic line in AbstractOutboxEventPublisher
find . -name "AbstractOutboxEventPublisher.java" -type f | head -5Repository: Puneethkumarck/stablebridge-platform
Length of output: 202
🏁 Script executed:
# Check RateLimitEvent structure
cat -n api-gateway-iam/api-gateway-iam/src/main/java/com/stablecoin/payments/gateway/iam/domain/model/RateLimitEvent.javaRepository: Puneethkumarck/stablebridge-platform
Length of output: 733
🏁 Script executed:
# Search for usages of AbstractOutboxEventPublisher
rg -n "AbstractOutboxEventPublisher" --type java -A 3 -B 3 | head -50Repository: Puneethkumarck/stablebridge-platform
Length of output: 8110
🏁 Script executed:
cat -n platform-infra/src/main/java/com/stablecoin/payments/platform/infrastructure/messaging/AbstractOutboxEventPublisher.javaRepository: Puneethkumarck/stablebridge-platform
Length of output: 1850
🏁 Script executed:
cat -n payment-orchestrator/payment-orchestrator/src/main/java/com/stablecoin/payments/orchestrator/infrastructure/messaging/OutboxEventPublisher.javaRepository: Puneethkumarck/stablebridge-platform
Length of output: 781
🏁 Script executed:
cat -n merchant-iam/merchant-iam/src/main/java/com/stablecoin/payments/merchant/iam/infrastructure/messaging/OutboxEventPublisher.javaRepository: Puneethkumarck/stablebridge-platform
Length of output: 771
🏁 Script executed:
cat -n fx-liquidity-engine/fx-liquidity-engine/src/main/java/com/stablecoin/payments/fx/infrastructure/messaging/OutboxEventPublisher.javaRepository: Puneethkumarck/stablebridge-platform
Length of output: 765
🏁 Script executed:
rg -n "RateLimitEvent" --type java | head -20Repository: Puneethkumarck/stablebridge-platform
Length of output: 4589
🏁 Script executed:
# Check if any POJO event classes are published through these OutboxEventPublisher implementations
rg -n "publish\(" --type java -B 5 | grep -E "(RateLimitEvent|@Getter)" -A 5 | head -40Repository: Puneethkumarck/stablebridge-platform
Length of output: 62
Code assumes record-style accessors; add safety for traditional POJOs or enforce records via documentation.
The reflection logic (line 28) calls getMethod(fieldName) expecting methods like paymentId(), not getPaymentId(). While all current events are records matching this pattern, the design is fragile. If traditional JavaBeans are added to the event model, reflection fails silently then throws IllegalArgumentException at runtime.
Consider either:
- Document that all events must be Java records, or
- Add fallback logic to try
get<FieldName>()pattern if the record accessor fails (e.g., capitalize first char of fieldName and prefix with "get")
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@platform-infra/src/main/java/com/stablecoin/payments/platform/infrastructure/messaging/AbstractOutboxEventPublisher.java`
at line 28, The reflection in AbstractOutboxEventPublisher calling
event.getClass().getMethod(fieldName) assumes record-style accessors and will
fail for POJOs; update the logic to attempt the record accessor first and if
NoSuchMethodException is thrown, construct the JavaBean getter name by
capitalizing the first character of fieldName and prefixing with "get" (e.g.,
"getPaymentId") and try event.getClass().getMethod(getterName), or alternatively
document that events must be records—ensure the code references and handles
exceptions around event.getClass().getMethod(fieldName) and uses the fallback
getter lookup when the record accessor is absent.
Comment on lines
+18
to
+19
| var topic = resolveStaticField(event, "TOPIC"); | ||
| var key = metadata.getKey(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
find . -type f -name "AbstractOutboxHandler.java" | head -5Repository: Puneethkumarck/stablebridge-platform
Length of output: 195
🏁 Script executed:
find . -type f -path "*/messaging/*" -name "*.java" | grep -i outbox | head -10Repository: Puneethkumarck/stablebridge-platform
Length of output: 1446
🏁 Script executed:
cat -n ./platform-infra/src/main/java/com/stablecoin/payments/platform/infrastructure/messaging/AbstractOutboxHandler.javaRepository: Puneethkumarck/stablebridge-platform
Length of output: 2168
🏁 Script executed:
# Search for event classes with TOPIC field
rg "public static final String TOPIC" --type javaRepository: Puneethkumarck/stablebridge-platform
Length of output: 8434
🏁 Script executed:
# Search for interfaces or marker interfaces that might enforce this
rg "interface.*Event|interface.*Outbox" --type java | head -20Repository: Puneethkumarck/stablebridge-platform
Length of output: 3771
Enforce TOPIC contract via marker interface or base class.
While all existing events declare public static final String TOPIC, the current reflection-based approach has no compile-time safety—a missing field fails at runtime after the outbox record is already persisted. Introduce an OutboxEvent marker interface (or base class) requiring the static field, preventing future non-conforming events from being added unknowingly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@platform-infra/src/main/java/com/stablecoin/payments/platform/infrastructure/messaging/AbstractOutboxHandler.java`
around lines 18 - 19, Introduce an OutboxEvent marker interface that exposes an
instance method String getTopic(), update all event classes to implement
OutboxEvent and have getTopic() return their existing TOPIC constant, and then
stop using reflection: in AbstractOutboxHandler replace
resolveStaticField(event, "TOPIC") with a cast to OutboxEvent and call
getTopic(); also update any method signatures or generics in
AbstractOutboxHandler that accept events to require OutboxEvent so
non-conforming events fail at compile time.
Comment on lines
+21
to
+30
| kafkaTemplate.send(topic, key, event).get(10, TimeUnit.SECONDS); | ||
| log.debug("Published outbox event type={} topic={} key={}", | ||
| event.getClass().getSimpleName(), topic, key); | ||
| } catch (InterruptedException e) { | ||
| Thread.currentThread().interrupt(); | ||
| throw new RuntimeException("Kafka send interrupted for event " + event.getClass().getSimpleName(), e); | ||
| } catch (Exception e) { | ||
| log.error("Failed to publish event type={} topic={}: {}", | ||
| event.getClass().getSimpleName(), topic, e.getMessage()); | ||
| throw new RuntimeException("Kafka send failed for event " + event.getClass().getSimpleName(), e); |
There was a problem hiding this comment.
Sanitize logging in the shared outbox handler.
metadata.getKey() and e.getMessage() can carry business identifiers or payload fragments. Emitting them from this base class turns one service-specific leak into a platform-wide one. Log event type/topic and a stable error classification only.
Suggested change
try {
kafkaTemplate.send(topic, key, event).get(10, TimeUnit.SECONDS);
- log.debug("Published outbox event type={} topic={} key={}",
- event.getClass().getSimpleName(), topic, key);
+ log.debug("Published outbox event type={} topic={}",
+ event.getClass().getSimpleName(), topic);
} catch (InterruptedException e) {
Thread.currentThread().interrupt();
throw new RuntimeException("Kafka send interrupted for event " + event.getClass().getSimpleName(), e);
} catch (Exception e) {
- log.error("Failed to publish event type={} topic={}: {}",
- event.getClass().getSimpleName(), topic, e.getMessage());
+ log.error("Failed to publish outbox event type={} topic={} errorType={}",
+ event.getClass().getSimpleName(), topic, e.getClass().getSimpleName());
throw new RuntimeException("Kafka send failed for event " + event.getClass().getSimpleName(), e);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@platform-infra/src/main/java/com/stablecoin/payments/platform/infrastructure/messaging/AbstractOutboxHandler.java`
around lines 21 - 30, The base outbox handler logs potentially sensitive values
(the message key and exception message) in log.debug and log.error; update
AbstractOutboxHandler to avoid emitting metadata.getKey()/key and
e.getMessage(): change the log.debug call after kafkaTemplate.send(...).get(...)
to only include event.getClass().getSimpleName() and topic, and change the
log.error in the generic catch to log event type, topic and a stable error
classification such as e.getClass().getSimpleName() or a fixed string like
"publish-failure" (do not include the exception message or key); keep
InterruptedException handling but ensure Thread.currentThread().interrupt() is
still called and the thrown RuntimeException wraps the original exception
without exposing sensitive strings in logs.
Comment on lines
+17
to
+30
| public static void verifyAll(String basePackage) { | ||
| JavaClasses importedClasses = new ClassFileImporter() | ||
| .withImportOption(ImportOption.Predefined.DO_NOT_INCLUDE_TESTS) | ||
| .importPackages(basePackage); | ||
|
|
||
| domainShouldNotDependOnSpring().check(importedClasses); | ||
| domainShouldNotDependOnJpa().check(importedClasses); | ||
| domainShouldNotDependOnInfrastructure().check(importedClasses); | ||
| domainShouldNotDependOnApplication().check(importedClasses); | ||
| infrastructureShouldNotDependOnController().check(importedClasses); | ||
| portsShouldBeInterfaces().check(importedClasses); | ||
| domainEventsShouldBeRecords().check(importedClasses); | ||
| controllersShouldResideInApplicationController().check(importedClasses); | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider aggregating rule violations for better DX.
Sequential check() calls stop at the first failure. Developers fixing architecture violations must iterate multiple times to discover all issues.
ArchUnit's CompositeArchRule.of(...) or collecting violations before throwing would surface all problems in one pass.
♻️ Optional: aggregate all violations
public static void verifyAll(String basePackage) {
JavaClasses importedClasses = new ClassFileImporter()
.withImportOption(ImportOption.Predefined.DO_NOT_INCLUDE_TESTS)
.importPackages(basePackage);
- domainShouldNotDependOnSpring().check(importedClasses);
- domainShouldNotDependOnJpa().check(importedClasses);
- domainShouldNotDependOnInfrastructure().check(importedClasses);
- domainShouldNotDependOnApplication().check(importedClasses);
- infrastructureShouldNotDependOnController().check(importedClasses);
- portsShouldBeInterfaces().check(importedClasses);
- domainEventsShouldBeRecords().check(importedClasses);
- controllersShouldResideInApplicationController().check(importedClasses);
+ CompositeArchRule.of(
+ domainShouldNotDependOnSpring(),
+ domainShouldNotDependOnJpa(),
+ domainShouldNotDependOnInfrastructure(),
+ domainShouldNotDependOnApplication(),
+ infrastructureShouldNotDependOnController(),
+ portsShouldBeInterfaces(),
+ domainEventsShouldBeRecords(),
+ controllersShouldResideInApplicationController()
+ ).check(importedClasses);
}Requires import: import com.tngtech.archunit.lang.CompositeArchRule;
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public static void verifyAll(String basePackage) { | |
| JavaClasses importedClasses = new ClassFileImporter() | |
| .withImportOption(ImportOption.Predefined.DO_NOT_INCLUDE_TESTS) | |
| .importPackages(basePackage); | |
| domainShouldNotDependOnSpring().check(importedClasses); | |
| domainShouldNotDependOnJpa().check(importedClasses); | |
| domainShouldNotDependOnInfrastructure().check(importedClasses); | |
| domainShouldNotDependOnApplication().check(importedClasses); | |
| infrastructureShouldNotDependOnController().check(importedClasses); | |
| portsShouldBeInterfaces().check(importedClasses); | |
| domainEventsShouldBeRecords().check(importedClasses); | |
| controllersShouldResideInApplicationController().check(importedClasses); | |
| } | |
| public static void verifyAll(String basePackage) { | |
| JavaClasses importedClasses = new ClassFileImporter() | |
| .withImportOption(ImportOption.Predefined.DO_NOT_INCLUDE_TESTS) | |
| .importPackages(basePackage); | |
| CompositeArchRule.of( | |
| domainShouldNotDependOnSpring(), | |
| domainShouldNotDependOnJpa(), | |
| domainShouldNotDependOnInfrastructure(), | |
| domainShouldNotDependOnApplication(), | |
| infrastructureShouldNotDependOnController(), | |
| portsShouldBeInterfaces(), | |
| domainEventsShouldBeRecords(), | |
| controllersShouldResideInApplicationController() | |
| ).check(importedClasses); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@platform-test/src/testFixtures/java/com/stablecoin/payments/platform/test/HexagonalArchitectureRules.java`
around lines 17 - 30, The verifyAll method currently calls each rule's check()
sequentially which stops at the first failure; change it to build a single
composite rule (using CompositeArchRule.of(...)) combining
domainShouldNotDependOnSpring(), domainShouldNotDependOnJpa(),
domainShouldNotDependOnInfrastructure(), domainShouldNotDependOnApplication(),
infrastructureShouldNotDependOnController(), portsShouldBeInterfaces(),
domainEventsShouldBeRecords(), controllersShouldResideInApplicationController()
and call check(importedClasses) once; add the import for
com.tngtech.archunit.lang.CompositeArchRule and replace the multiple check()
invocations in verifyAll with the composite rule check to surface all violations
in one run.
Comment on lines
+10
to
+79
| public final class TestContainerSupport { | ||
|
|
||
| private TestContainerSupport() {} | ||
|
|
||
| @SuppressWarnings("resource") | ||
| public static PostgreSQLContainer<?> postgres(String databaseName) { | ||
| return new PostgreSQLContainer<>("postgres:16-alpine") | ||
| .withDatabaseName(databaseName) | ||
| .withUsername("test") | ||
| .withPassword("test"); | ||
| } | ||
|
|
||
| public static KafkaContainer kafka() { | ||
| return new KafkaContainer(DockerImageName.parse("confluentinc/cp-kafka:7.6.0")); | ||
| } | ||
|
|
||
| public static GenericContainer<?> redis() { | ||
| return new GenericContainer<>(DockerImageName.parse("redis:7-alpine")) | ||
| .withExposedPorts(6379); | ||
| } | ||
|
|
||
| public static void startAll(Startable... containers) { | ||
| for (Startable container : containers) { | ||
| try { | ||
| container.start(); | ||
| } catch (RuntimeException ex) { | ||
| safeStop(containers); | ||
| throw ex; | ||
| } | ||
| } | ||
| registerShutdownHook(containers); | ||
| } | ||
|
|
||
| public static void safeStop(Startable... containers) { | ||
| for (Startable container : containers) { | ||
| try { | ||
| if (container != null) { | ||
| container.stop(); | ||
| } | ||
| } catch (Exception ignored) { | ||
| // best-effort cleanup | ||
| } | ||
| } | ||
| } | ||
|
|
||
| public static void registerShutdownHook(Startable... containers) { | ||
| Runtime.getRuntime().addShutdownHook(new Thread( | ||
| () -> safeStop(containers), | ||
| "testcontainers-shutdown" | ||
| )); | ||
| } | ||
|
|
||
| public static void registerPostgresProperties(DynamicPropertyRegistry registry, | ||
| PostgreSQLContainer<?> postgres) { | ||
| registry.add("spring.datasource.url", postgres::getJdbcUrl); | ||
| registry.add("spring.datasource.username", postgres::getUsername); | ||
| registry.add("spring.datasource.password", postgres::getPassword); | ||
| } | ||
|
|
||
| public static void registerKafkaProperties(DynamicPropertyRegistry registry, | ||
| KafkaContainer kafka) { | ||
| registry.add("spring.kafka.bootstrap-servers", kafka::getBootstrapServers); | ||
| registry.add("spring.cloud.stream.kafka.binder.brokers", kafka::getBootstrapServers); | ||
| } | ||
|
|
||
| public static void registerRedisProperties(DynamicPropertyRegistry registry, | ||
| GenericContainer<?> redis) { | ||
| registry.add("spring.data.redis.host", redis::getHost); | ||
| registry.add("spring.data.redis.port", () -> redis.getMappedPort(6379)); | ||
| } |
There was a problem hiding this comment.
Add focused coverage for the new fixture helpers.
This shared class now underpins multiple integration suites, but the nine new public methods land without direct tests for the container factory defaults, property registration, or the startAll cleanup path. A regression here will fan out across services. As per coding guidelines, "Every new public method should have at least one test".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@platform-test/src/testFixtures/java/com/stablecoin/payments/platform/test/TestContainerSupport.java`
around lines 10 - 79, Add unit tests for the new public helpers: verify
postgres(String) returns a PostgreSQLContainer configured with image
"postgres:16-alpine" and the provided databaseName/username/password (test
PostgreSQLContainer<?> postgres(String)), verify kafka() uses
"confluentinc/cp-kafka:7.6.0" (test KafkaContainer kafka()), and verify redis()
exposes port 6379 (test GenericContainer<?> redis()). Add tests for
registerPostgresProperties, registerKafkaProperties and registerRedisProperties
by passing a mock or test DynamicPropertyRegistry and asserting the expected
keys map to the container getters (postgres::getJdbcUrl,
kafka::getBootstrapServers, redis::getHost/getMappedPort). Add a startAll
cleanup test that supplies multiple Startable mocks where one throws on start
and assert safeStop was called on the others and the original RuntimeException
is propagated (use Mockito spies/mocks to verify stop() calls); also add a
safeStop test that ensures exceptions from stop() are swallowed (mock a
Startable that throws on stop and assert no exception escapes).
… test exceptions (STA-202) - BaseGlobalExceptionHandler: sanitize validation logs (field count only), remove HandlerMethodValidationException (conflicts with service overrides) - AbstractOutboxEventPublisher: null key guard - AbstractOutboxHandler: PII-safety comment on error logs - Externalize namastack version to gradle.properties - merchant-iam/merchant-onboarding ArchitectureTest: skip JPA rule (domain entities legitimately use @JdbcTypeCode for enum mapping) - blockchain-custody: flush Redis between ITs - Convention plugin: remove failOnNoDiscoveredTests from integrationTest - payment-orchestrator: remove redundant spring-boot-starter-kafka Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (5)
platform-infra/src/main/java/com/stablecoin/payments/platform/infrastructure/messaging/AbstractOutboxEventPublisher.java (1)
28-34:⚠️ Potential issue | 🟠 MajorReflection still assumes record-style accessors only.
Line 28 resolves only
fieldName()methods. POJO events withget<FieldName>()will fail at runtime. Either add JavaBean getter fallback or explicitly enforce/document “records-only” event contracts in this base class.#!/bin/bash set -euo pipefail echo "== Publishers extending AbstractOutboxEventPublisher ==" rg -n --type=java -C2 'class\s+\w+\s+extends\s+AbstractOutboxEventPublisher' echo echo "== publish(...) call sites ==" rg -n --type=java -C2 '\.publish\(' echo echo "== Event declarations (record vs class) ==" rg -n --type=java '^\s*(public\s+)?(record|class)\s+\w+Event\b' echo echo "== JavaBean-style getters on Event types ==" rg -n --type=java '^\s*(public\s+)?[[:alnum:]_<>, ?\[\]]+\s+get[A-Z][A-Za-z0-9_]*\s*\(\s*\)'Expected result: if published event payloads include non-record classes or JavaBean-only getters, add getter fallback in
resolveKey; otherwise, codify records-only requirement in Javadoc/tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platform-infra/src/main/java/com/stablecoin/payments/platform/infrastructure/messaging/AbstractOutboxEventPublisher.java` around lines 28 - 34, The resolveKey logic in AbstractOutboxEventPublisher currently only tries record-style accessors (event.getClass().getMethod(fieldName)) so POJO events with JavaBean getters fail; update resolveKey to catch NoSuchMethodException and then attempt the JavaBean getter name "get" + capitalize(fieldName) via getMethod, invoke it and return its string value if present, otherwise fall back to the existing behavior (or null); alternatively, if you prefer a stricter contract, add clear Javadoc to AbstractOutboxEventPublisher and unit tests for resolveKey documenting that only record-style events are supported and remove the JavaBean fallback—choose one approach and implement the corresponding change around resolveKey and its callers.platform-infra/src/main/java/com/stablecoin/payments/platform/infrastructure/exception/BaseGlobalExceptionHandler.java (2)
57-57:⚠️ Potential issue | 🟠 MajorRemove raw exception messages from shared INFO logs.
Logging
ex.getMessage()at these points can leak request values/secrets into centralized logs. Keep parameter/context only, without raw payload-derived message text.Proposed fix
- log.info("Type mismatch for parameter '{}': {}", ex.getName(), ex.getMessage()); + log.info("Type mismatch for parameter '{}'", ex.getName()); ... - log.info("Illegal argument: {}", ex.getMessage()); + log.info("Illegal argument while handling request"); ... - log.info("Invalid state: {}", ex.getMessage()); + log.info("Invalid state while handling request");Also applies to: 66-66, 74-74
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platform-infra/src/main/java/com/stablecoin/payments/platform/infrastructure/exception/BaseGlobalExceptionHandler.java` at line 57, Remove raw exception messages from INFO logs in BaseGlobalExceptionHandler: replace log.info calls that include ex.getMessage() with messages that only reference parameter/context (e.g., keep ex.getName() or other safe identifiers) so no payload-derived text is logged; update the log.info call shown (the one using ex.getName() and ex.getMessage()) as well as the similar logging statements at the other occurrences (lines referenced in the review) to only include safe context, and leave exception details to secure/error-level logs or attach the exception object (not its message) where appropriate.
32-38:⚠️ Potential issue | 🟠 MajorInclude class-level (
ObjectError) validation failures inerrors.
getFieldErrors()excludes bean/cross-field constraints, so shared 400 payloads can be incomplete. Aggregate fromgetAllErrors()and map non-field errors to a global key.Proposed fix
- var errors = ex.getBindingResult().getFieldErrors().stream() - .collect(Collectors.groupingBy( - FieldError::getField, - Collectors.mapping(ObjectError::getDefaultMessage, Collectors.toList()))); + var errors = ex.getBindingResult().getAllErrors().stream() + .collect(Collectors.groupingBy( + err -> (err instanceof FieldError fe) ? fe.getField() : "_global", + Collectors.mapping(ObjectError::getDefaultMessage, Collectors.toList())));#!/bin/bash # Verify current aggregation source in the shared handler. rg -n "getFieldErrors\\(|getAllErrors\\(|getGlobalErrors\\(" platform-infra/src/main/java/com/stablecoin/payments/platform/infrastructure/exception/BaseGlobalExceptionHandler.java🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platform-infra/src/main/java/com/stablecoin/payments/platform/infrastructure/exception/BaseGlobalExceptionHandler.java` around lines 32 - 38, In BaseGlobalExceptionHandler, replace the existing use of ex.getBindingResult().getFieldErrors() with aggregation from ex.getBindingResult().getAllErrors(): collect errors by grouping FieldError instances by FieldError::getField and mapping their messages, and map non-FieldError/ObjectError (bean/cross-field) instances to a shared key such as "global" (or "") with their ObjectError::getDefaultMessage; update the log to use the total error count and return ApiError.withErrors(code("0001"), BAD_REQUEST.getReasonPhrase(), "Invalid request content", errors) using this combined map so bean-level validation messages are included.platform-infra/src/main/java/com/stablecoin/payments/platform/infrastructure/messaging/AbstractOutboxHandler.java (2)
19-23:⚠️ Potential issue | 🟠 MajorSanitize the shared outbox logs.
metadata.getKey()is derived from service event accessors upstream inplatform-infra/src/main/java/com/stablecoin/payments/platform/infrastructure/messaging/AbstractOutboxEventPublisher.java, Lines 10-42, so it can carry business identifiers. Emitting that key on success ande.getMessage()on failure makes this base handler a platform-wide leak, and the inline “safe to log” assumption is too strong for generic Kafka/serializer failures. LogeventType,topic, and a stableerrorTypeonly.Minimal fix
- log.debug("Published outbox event type={} topic={} key={}", - event.getClass().getSimpleName(), topic, key); + log.debug("Published outbox event type={} topic={}", + event.getClass().getSimpleName(), topic); ... - // Kafka errors don't contain user PII — safe to log - log.error("Failed to publish event type={} topic={}: {}", - event.getClass().getSimpleName(), topic, e.getMessage()); + log.error("Failed to publish outbox event type={} topic={} errorType={}", + event.getClass().getSimpleName(), topic, e.getClass().getSimpleName());Also applies to: 27-30
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platform-infra/src/main/java/com/stablecoin/payments/platform/infrastructure/messaging/AbstractOutboxHandler.java` around lines 19 - 23, In AbstractOutboxHandler, avoid logging potentially sensitive business identifiers like metadata.getKey() and exception messages; update the success and failure log calls around kafkaTemplate.send(topic, key, event).get(...) so they only emit the event type (event.getClass().getSimpleName()), topic, and a stable errorType/string (e.g., "KAFKA_SEND_FAILURE") instead of the raw key or e.getMessage(); remove any references to metadata.getKey() from log.debug/log.error and apply the same change to the analogous block that covers lines 27-30 to ensure consistent sanitized logs across the class.
17-18: 🛠️ Refactor suggestion | 🟠 MajorMake topic resolution part of the type system.
handle(Object, ...)plus reflectiveTOPIClookup defers a contract violation to runtime, after the outbox row is already persisted. In shared infrastructure, that is an avoidable poison-message path. Require a dedicated event interface/base type withgetTopic()and bindhandle(...)to that type instead ofObject.Minimal direction
- public void handle(Object event, OutboxRecordMetadata metadata) { - var topic = resolveStaticField(event, "TOPIC"); + public void handle(OutboxEvent event, OutboxRecordMetadata metadata) { + var topic = event.getTopic();public interface OutboxEvent { String getTopic(); }Also applies to: 35-40
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@platform-infra/src/main/java/com/stablecoin/payments/platform/infrastructure/messaging/AbstractOutboxHandler.java` around lines 17 - 18, The current AbstractOutboxHandler.handle(Object, OutboxRecordMetadata) uses reflective lookup via resolveStaticField(event, "TOPIC"), which defers a contract violation to runtime; introduce a typed contract (e.g., create an OutboxEvent interface with String getTopic()) and change the handler signature to handle(OutboxEvent event, OutboxRecordMetadata metadata), remove resolveStaticField usage and call event.getTopic() instead, and update all call sites and subclasses of AbstractOutboxHandler to accept/emit OutboxEvent implementations so topic resolution is compile-time enforced; ensure any other code paths mentioned (lines ~35-40) that rely on TOPIC reflection are similarly migrated to use getTopic().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@blockchain-custody/blockchain-custody/src/integration-test/java/com/stablecoin/payments/custody/AbstractIntegrationTest.java`:
- Around line 59-63: The Redis flush code can NPE if
redisTemplate.getConnectionFactory() is null and may leak the connection if
flushAll() throws; update the block that calls
redisTemplate.getConnectionFactory().getConnection() to first check for a
non-null ConnectionFactory (redisTemplate.getConnectionFactory()) and only
proceed if present, then obtain the connection via getConnection() inside a
try-with-resources (or try/finally) so connection.close() always runs, and call
connection.serverCommands().flushAll() inside that safe scope (reference:
redisTemplate, getConnectionFactory(), getConnection(),
connection.serverCommands().flushAll(), connection.close()).
- Around line 30-35: The field visibility is inconsistent: POSTGRES is
package-private while KAFKA and REDIS are protected; in AbstractIntegrationTest
change the POSTGRES declaration to protected (matching KAFKA and REDIS) so
subclasses can access all containers consistently, i.e., update the POSTGRES
symbol's modifier to protected to align with KAFKA and REDIS.
In `@buildSrc/src/main/kotlin/stablebridge.service.gradle.kts`:
- Around line 177-180: The MapStruct compiler arg currently uses
"-Amapstruct.unmappedTargetPolicy=IGNORE" which silently drops unmapped fields;
change this to "-Amapstruct.unmappedTargetPolicy=WARN" in the
options.compilerArgs list (or add a nearby comment explaining why IGNORE is
intentionally safe for this project) so unmapped mappings surface as compiler
warnings instead of being suppressed; update the entry where
options.compilerArgs.addAll(...) is defined to reflect the new value or document
the rationale.
In
`@merchant-onboarding/merchant-onboarding/src/test/java/com/stablecoin/payments/merchant/onboarding/arch/ArchitectureTest.java`:
- Around line 23-63: Add a test that asserts the outbox pattern by checking
presence and relationships of OutboxEvent, OutboxEventRepository, and
OutboxRelayJob: implement a new rule method (e.g.,
HexagonalArchitectureRules.outboxPatternShouldExist() or outboxPatternRule())
that verifies an OutboxEvent entity exists, an OutboxEventRepository interface
handles persistence, and an OutboxRelayJob class/process depends on that
repository (and/or schedules relay), then call that rule from ArchitectureTest
(e.g., add a test method domainShouldHaveOutboxPattern() that calls
HexagonalArchitectureRules.outboxPatternShouldExist().check(importedClasses)).
Ensure the rule uses ArchUnit predicates referencing the symbols OutboxEvent,
OutboxEventRepository, and OutboxRelayJob.
In `@payment-orchestrator/payment-orchestrator/build.gradle.kts`:
- Around line 5-6: Replace the project properties temporalVersion and
wiremockVersion with a version-catalog entry: add temporal = "x.y.z" and
wiremock = "x.y.z" under [versions] in libs.versions.toml, remove the val
temporalVersion: String by project and val wiremockVersion: String by project
declarations in build.gradle.kts, and update any usage to reference the catalog
(e.g., use libs.versions.temporal.get() and libs.versions.wiremock.get() or the
generated type-safe accessors) so all modules use the centralized versions.
In
`@platform-infra/src/main/java/com/stablecoin/payments/platform/infrastructure/exception/BaseGlobalExceptionHandler.java`:
- Around line 25-27: The helper method code(String suffix) must guard against a
null or blank errorCodePrefix() to avoid producing malformed shared codes;
update code(...) to call errorCodePrefix(), trim and validate the result, and if
it's null/blank throw an IllegalStateException (or a clear runtime exception)
with a message referencing errorCodePrefix() and code(String) so
misconfiguration is caught early, otherwise build the code using the validated
prefix and the provided suffix.
- Around line 44-50: The log in BaseGlobalExceptionHandler currently logs
errors.size() which is the number of grouped property keys, not the total number
of constraint violations; update the logging to compute the real total (e.g.,
sum the sizes of the lists in the grouped map produced from
ex.getConstraintViolations()) and log that total count (or both the number of
properties and total violations) before returning ApiError.withErrors; refer to
the variables/methods errors, ex.getConstraintViolations(), and
ApiError.withErrors to locate the change.
In
`@platform-infra/src/main/java/com/stablecoin/payments/platform/infrastructure/messaging/AbstractOutboxEventPublisher.java`:
- Around line 19-20: Add an explicit null guard at the API boundary inside the
publish method of AbstractOutboxEventPublisher: at the very start of
publish(Object event) call Objects.requireNonNull(event, "event must not be
null") (or throw a clear IllegalArgumentException) so resolveKey(event) and
subsequent logging never see a null and the failure message is descriptive;
ensure you add the necessary import for java.util.Objects if using
requireNonNull.
---
Duplicate comments:
In
`@platform-infra/src/main/java/com/stablecoin/payments/platform/infrastructure/exception/BaseGlobalExceptionHandler.java`:
- Line 57: Remove raw exception messages from INFO logs in
BaseGlobalExceptionHandler: replace log.info calls that include ex.getMessage()
with messages that only reference parameter/context (e.g., keep ex.getName() or
other safe identifiers) so no payload-derived text is logged; update the
log.info call shown (the one using ex.getName() and ex.getMessage()) as well as
the similar logging statements at the other occurrences (lines referenced in the
review) to only include safe context, and leave exception details to
secure/error-level logs or attach the exception object (not its message) where
appropriate.
- Around line 32-38: In BaseGlobalExceptionHandler, replace the existing use of
ex.getBindingResult().getFieldErrors() with aggregation from
ex.getBindingResult().getAllErrors(): collect errors by grouping FieldError
instances by FieldError::getField and mapping their messages, and map
non-FieldError/ObjectError (bean/cross-field) instances to a shared key such as
"global" (or "") with their ObjectError::getDefaultMessage; update the log to
use the total error count and return ApiError.withErrors(code("0001"),
BAD_REQUEST.getReasonPhrase(), "Invalid request content", errors) using this
combined map so bean-level validation messages are included.
In
`@platform-infra/src/main/java/com/stablecoin/payments/platform/infrastructure/messaging/AbstractOutboxEventPublisher.java`:
- Around line 28-34: The resolveKey logic in AbstractOutboxEventPublisher
currently only tries record-style accessors
(event.getClass().getMethod(fieldName)) so POJO events with JavaBean getters
fail; update resolveKey to catch NoSuchMethodException and then attempt the
JavaBean getter name "get" + capitalize(fieldName) via getMethod, invoke it and
return its string value if present, otherwise fall back to the existing behavior
(or null); alternatively, if you prefer a stricter contract, add clear Javadoc
to AbstractOutboxEventPublisher and unit tests for resolveKey documenting that
only record-style events are supported and remove the JavaBean fallback—choose
one approach and implement the corresponding change around resolveKey and its
callers.
In
`@platform-infra/src/main/java/com/stablecoin/payments/platform/infrastructure/messaging/AbstractOutboxHandler.java`:
- Around line 19-23: In AbstractOutboxHandler, avoid logging potentially
sensitive business identifiers like metadata.getKey() and exception messages;
update the success and failure log calls around kafkaTemplate.send(topic, key,
event).get(...) so they only emit the event type
(event.getClass().getSimpleName()), topic, and a stable errorType/string (e.g.,
"KAFKA_SEND_FAILURE") instead of the raw key or e.getMessage(); remove any
references to metadata.getKey() from log.debug/log.error and apply the same
change to the analogous block that covers lines 27-30 to ensure consistent
sanitized logs across the class.
- Around line 17-18: The current AbstractOutboxHandler.handle(Object,
OutboxRecordMetadata) uses reflective lookup via resolveStaticField(event,
"TOPIC"), which defers a contract violation to runtime; introduce a typed
contract (e.g., create an OutboxEvent interface with String getTopic()) and
change the handler signature to handle(OutboxEvent event, OutboxRecordMetadata
metadata), remove resolveStaticField usage and call event.getTopic() instead,
and update all call sites and subclasses of AbstractOutboxHandler to accept/emit
OutboxEvent implementations so topic resolution is compile-time enforced; ensure
any other code paths mentioned (lines ~35-40) that rely on TOPIC reflection are
similarly migrated to use getTopic().
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0e381a97-29ca-434e-8a03-757e94e5bbf1
📒 Files selected for processing (10)
blockchain-custody/blockchain-custody/src/integration-test/java/com/stablecoin/payments/custody/AbstractIntegrationTest.javabuildSrc/src/main/kotlin/stablebridge.service.gradle.ktsgradle.propertiesmerchant-iam/merchant-iam/src/test/java/com/stablecoin/payments/merchant/iam/arch/ArchitectureTest.javamerchant-onboarding/merchant-onboarding/src/test/java/com/stablecoin/payments/merchant/onboarding/arch/ArchitectureTest.javapayment-orchestrator/payment-orchestrator/build.gradle.ktsplatform-infra/build.gradle.ktsplatform-infra/src/main/java/com/stablecoin/payments/platform/infrastructure/exception/BaseGlobalExceptionHandler.javaplatform-infra/src/main/java/com/stablecoin/payments/platform/infrastructure/messaging/AbstractOutboxEventPublisher.javaplatform-infra/src/main/java/com/stablecoin/payments/platform/infrastructure/messaging/AbstractOutboxHandler.java
Comment on lines
+30
to
+35
| static final PostgreSQLContainer<?> POSTGRES = postgres("s4_blockchain_custody"); | ||
| protected static final KafkaContainer KAFKA = kafka(); | ||
| protected static final GenericContainer<?> REDIS = redis(); | ||
|
|
||
| static { | ||
| try { | ||
| POSTGRES.start(); | ||
| KAFKA.start(); | ||
| } catch (RuntimeException ex) { | ||
| safeStop(KAFKA); | ||
| safeStop(POSTGRES); | ||
| throw ex; | ||
| } | ||
| Runtime.getRuntime().addShutdownHook(new Thread(() -> { | ||
| safeStop(KAFKA); | ||
| safeStop(POSTGRES); | ||
| }, "testcontainers-shutdown")); | ||
| } | ||
|
|
||
| private static void safeStop(Startable container) { | ||
| try { | ||
| if (container != null) { | ||
| container.stop(); | ||
| } | ||
| } catch (Exception ignored) { | ||
| // best-effort cleanup | ||
| } | ||
| startAll(POSTGRES, KAFKA, REDIS); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Visibility inconsistency on container fields.
POSTGRES is package-private while KAFKA and REDIS are protected. Consider aligning visibility for consistency—likely all should be protected if subclasses in other packages need access.
♻️ Suggested fix
- static final PostgreSQLContainer<?> POSTGRES = postgres("s4_blockchain_custody");
+ protected static final PostgreSQLContainer<?> POSTGRES = postgres("s4_blockchain_custody");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| static final PostgreSQLContainer<?> POSTGRES = postgres("s4_blockchain_custody"); | |
| protected static final KafkaContainer KAFKA = kafka(); | |
| protected static final GenericContainer<?> REDIS = redis(); | |
| static { | |
| try { | |
| POSTGRES.start(); | |
| KAFKA.start(); | |
| } catch (RuntimeException ex) { | |
| safeStop(KAFKA); | |
| safeStop(POSTGRES); | |
| throw ex; | |
| } | |
| Runtime.getRuntime().addShutdownHook(new Thread(() -> { | |
| safeStop(KAFKA); | |
| safeStop(POSTGRES); | |
| }, "testcontainers-shutdown")); | |
| } | |
| private static void safeStop(Startable container) { | |
| try { | |
| if (container != null) { | |
| container.stop(); | |
| } | |
| } catch (Exception ignored) { | |
| // best-effort cleanup | |
| } | |
| startAll(POSTGRES, KAFKA, REDIS); | |
| protected static final PostgreSQLContainer<?> POSTGRES = postgres("s4_blockchain_custody"); | |
| protected static final KafkaContainer KAFKA = kafka(); | |
| protected static final GenericContainer<?> REDIS = redis(); | |
| static { | |
| startAll(POSTGRES, KAFKA, REDIS); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@blockchain-custody/blockchain-custody/src/integration-test/java/com/stablecoin/payments/custody/AbstractIntegrationTest.java`
around lines 30 - 35, The field visibility is inconsistent: POSTGRES is
package-private while KAFKA and REDIS are protected; in AbstractIntegrationTest
change the POSTGRES declaration to protected (matching KAFKA and REDIS) so
subclasses can access all containers consistently, i.e., update the POSTGRES
symbol's modifier to protected to align with KAFKA and REDIS.
Comment on lines
+59
to
+63
|
|
||
| // Flush Redis cache between tests | ||
| var connection = redisTemplate.getConnectionFactory().getConnection(); | ||
| connection.serverCommands().flushAll(); | ||
| connection.close(); |
There was a problem hiding this comment.
Guard against NPE and resource leak in Redis flush.
getConnectionFactory() can return null, and if flushAll() throws, the connection won't be closed. Use try-with-resources for safe cleanup.
🛡️ Proposed fix
// Flush Redis cache between tests
- var connection = redisTemplate.getConnectionFactory().getConnection();
- connection.serverCommands().flushAll();
- connection.close();
+ var connectionFactory = redisTemplate.getConnectionFactory();
+ if (connectionFactory != null) {
+ try (var connection = connectionFactory.getConnection()) {
+ connection.serverCommands().flushAll();
+ }
+ }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@blockchain-custody/blockchain-custody/src/integration-test/java/com/stablecoin/payments/custody/AbstractIntegrationTest.java`
around lines 59 - 63, The Redis flush code can NPE if
redisTemplate.getConnectionFactory() is null and may leak the connection if
flushAll() throws; update the block that calls
redisTemplate.getConnectionFactory().getConnection() to first check for a
non-null ConnectionFactory (redisTemplate.getConnectionFactory()) and only
proceed if present, then obtain the connection via getConnection() inside a
try-with-resources (or try/finally) so connection.close() always runs, and call
connection.serverCommands().flushAll() inside that safe scope (reference:
redisTemplate, getConnectionFactory(), getConnection(),
connection.serverCommands().flushAll(), connection.close()).
Comment on lines
+177
to
+180
| options.compilerArgs.addAll(listOf( | ||
| "-Amapstruct.defaultComponentModel=spring", | ||
| "-Amapstruct.unmappedTargetPolicy=IGNORE" | ||
| )) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
unmappedTargetPolicy=IGNORE silently drops unmapped fields.
This can hide legitimate mapping bugs. Consider WARN to surface issues during compilation without failing the build, or document why IGNORE is acceptable project-wide.
♻️ Proposed change
tasks.withType<JavaCompile> {
options.compilerArgs.addAll(listOf(
"-Amapstruct.defaultComponentModel=spring",
- "-Amapstruct.unmappedTargetPolicy=IGNORE"
+ "-Amapstruct.unmappedTargetPolicy=WARN"
))
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| options.compilerArgs.addAll(listOf( | |
| "-Amapstruct.defaultComponentModel=spring", | |
| "-Amapstruct.unmappedTargetPolicy=IGNORE" | |
| )) | |
| tasks.withType<JavaCompile> { | |
| options.compilerArgs.addAll(listOf( | |
| "-Amapstruct.defaultComponentModel=spring", | |
| "-Amapstruct.unmappedTargetPolicy=WARN" | |
| )) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@buildSrc/src/main/kotlin/stablebridge.service.gradle.kts` around lines 177 -
180, The MapStruct compiler arg currently uses
"-Amapstruct.unmappedTargetPolicy=IGNORE" which silently drops unmapped fields;
change this to "-Amapstruct.unmappedTargetPolicy=WARN" in the
options.compilerArgs list (or add a nearby comment explaining why IGNORE is
intentionally safe for this project) so unmapped mappings surface as compiler
warnings instead of being suppressed; update the entry where
options.compilerArgs.addAll(...) is defined to reflect the new value or document
the rationale.
Comment on lines
+23
to
63
| @Test | ||
| @DisplayName("Domain should not depend on Spring (except stereotype and transaction)") | ||
| void domainShouldNotDependOnSpring() { | ||
| HexagonalArchitectureRules.domainShouldNotDependOnSpring().check(importedClasses); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("Domain should not depend on infrastructure") | ||
| void domainShouldNotDependOnInfrastructure() { | ||
| HexagonalArchitectureRules.domainShouldNotDependOnInfrastructure().check(importedClasses); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("domain must not depend on infrastructure") | ||
| void domainMustNotDependOnInfrastructure() { | ||
| ArchRule rule = noClasses() | ||
| .that().resideInAPackage(BASE + ".domain..") | ||
| .should().dependOnClassesThat() | ||
| .resideInAPackage(BASE + ".infrastructure.."); | ||
| rule.check(classes); | ||
| @DisplayName("Domain should not depend on application layer") | ||
| void domainShouldNotDependOnApplication() { | ||
| HexagonalArchitectureRules.domainShouldNotDependOnApplication().check(importedClasses); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("domain must not depend on application layer") | ||
| void domainMustNotDependOnApplication() { | ||
| ArchRule rule = noClasses() | ||
| .that().resideInAPackage(BASE + ".domain..") | ||
| .should().dependOnClassesThat() | ||
| .resideInAPackage(BASE + ".application.."); | ||
| rule.check(classes); | ||
| @DisplayName("Infrastructure should not depend on application controller") | ||
| void infrastructureShouldNotDependOnController() { | ||
| HexagonalArchitectureRules.infrastructureShouldNotDependOnController().check(importedClasses); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("domain must not import Spring Framework classes except stereotype annotations") | ||
| void domainMustNotImportSpring() { | ||
| ArchRule rule = noClasses() | ||
| .that().resideInAPackage(BASE + ".domain..") | ||
| .should().dependOnClassesThat( | ||
| resideInAPackage("org.springframework..") | ||
| .and(resideOutsideOfPackage("org.springframework.stereotype..")) | ||
| .and(resideOutsideOfPackage("org.springframework.transaction..")) | ||
| ); | ||
| rule.check(classes); | ||
| @DisplayName("Ports should be interfaces") | ||
| void portsShouldBeInterfaces() { | ||
| HexagonalArchitectureRules.portsShouldBeInterfaces().check(importedClasses); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("domain must not import Jakarta Persistence classes") | ||
| void domainMustNotImportJpa() { | ||
| ArchRule rule = noClasses() | ||
| .that().resideInAPackage(BASE + ".domain..") | ||
| .should().dependOnClassesThat() | ||
| .resideInAPackage("jakarta.persistence.."); | ||
| rule.check(classes); | ||
| @DisplayName("Domain events should be records") | ||
| void domainEventsShouldBeRecords() { | ||
| HexagonalArchitectureRules.domainEventsShouldBeRecords().check(importedClasses); | ||
| } | ||
|
|
||
| @Test | ||
| @DisplayName("infrastructure must not depend on application controller") | ||
| void infrastructureMustNotDependOnApplicationController() { | ||
| ArchRule rule = noClasses() | ||
| .that().resideInAPackage(BASE + ".infrastructure..") | ||
| .should().dependOnClassesThat() | ||
| .resideInAPackage(BASE + ".application.controller.."); | ||
| rule.check(classes); | ||
| @DisplayName("Controllers should reside in application.controller package") | ||
| void controllersShouldResideInApplicationController() { | ||
| HexagonalArchitectureRules.controllersShouldResideInApplicationController().check(importedClasses); | ||
| } |
There was a problem hiding this comment.
Missing architecture assertion for the required outbox pattern.
Lines 23–63 validate layer boundaries, but there is no test enforcing OutboxEvent, OutboxEventRepository, and OutboxRelayJob. Add a dedicated ArchUnit/shared-rule assertion for these artifacts.
As per coding guidelines, merchant-onboarding/**: Check for the outbox pattern: OutboxEvent entity + OutboxEventRepository + OutboxRelayJob.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@merchant-onboarding/merchant-onboarding/src/test/java/com/stablecoin/payments/merchant/onboarding/arch/ArchitectureTest.java`
around lines 23 - 63, Add a test that asserts the outbox pattern by checking
presence and relationships of OutboxEvent, OutboxEventRepository, and
OutboxRelayJob: implement a new rule method (e.g.,
HexagonalArchitectureRules.outboxPatternShouldExist() or outboxPatternRule())
that verifies an OutboxEvent entity exists, an OutboxEventRepository interface
handles persistence, and an OutboxRelayJob class/process depends on that
repository (and/or schedules relay), then call that rule from ArchitectureTest
(e.g., add a test method domainShouldHaveOutboxPattern() that calls
HexagonalArchitectureRules.outboxPatternShouldExist().check(importedClasses)).
Ensure the rule uses ArchUnit predicates referencing the symbols OutboxEvent,
OutboxEventRepository, and OutboxRelayJob.
Comment on lines
5
to
6
| val temporalVersion: String by project | ||
| val flywayVersion: String by project | ||
| val archunitVersion: String by project | ||
| val testcontainersVersion: String by project | ||
| val wiremockVersion: String by project |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Consider migrating versions to libs.versions.toml.
temporalVersion and wiremockVersion are externalized to gradle.properties, which works fine. However, for consistency across ~30 modules and better IDE support, consider centralizing these in a version catalog (libs.versions.toml).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@payment-orchestrator/payment-orchestrator/build.gradle.kts` around lines 5 -
6, Replace the project properties temporalVersion and wiremockVersion with a
version-catalog entry: add temporal = "x.y.z" and wiremock = "x.y.z" under
[versions] in libs.versions.toml, remove the val temporalVersion: String by
project and val wiremockVersion: String by project declarations in
build.gradle.kts, and update any usage to reference the catalog (e.g., use
libs.versions.temporal.get() and libs.versions.wiremock.get() or the generated
type-safe accessors) so all modules use the centralized versions.
Comment on lines
+25
to
+27
| private String code(String suffix) { | ||
| return errorCodePrefix() + "-" + suffix; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Guard errorCodePrefix() to prevent malformed shared error codes.
A null/blank prefix yields invalid codes and impacts all adopting services. Add a defensive check in code(...).
Proposed fix
+import java.util.Objects;
...
private String code(String suffix) {
- return errorCodePrefix() + "-" + suffix;
+ String prefix = Objects.requireNonNull(errorCodePrefix(), "errorCodePrefix must not be null");
+ if (prefix.isBlank()) {
+ throw new IllegalStateException("errorCodePrefix must not be blank");
+ }
+ return prefix + "-" + suffix;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@platform-infra/src/main/java/com/stablecoin/payments/platform/infrastructure/exception/BaseGlobalExceptionHandler.java`
around lines 25 - 27, The helper method code(String suffix) must guard against a
null or blank errorCodePrefix() to avoid producing malformed shared codes;
update code(...) to call errorCodePrefix(), trim and validate the result, and if
it's null/blank throw an IllegalStateException (or a clear runtime exception)
with a message referencing errorCodePrefix() and code(String) so
misconfiguration is caught early, otherwise build the code using the validated
prefix and the provided suffix.
Comment on lines
+44
to
+50
| var errors = ex.getConstraintViolations().stream() | ||
| .collect(Collectors.groupingBy( | ||
| v -> v.getPropertyPath().toString(), | ||
| Collectors.mapping(jakarta.validation.ConstraintViolation::getMessage, | ||
| Collectors.toList()))); | ||
| log.info("Constraint violation: {} violations", errors.size()); | ||
| return ApiError.withErrors(code("0001"), BAD_REQUEST.getReasonPhrase(), |
There was a problem hiding this comment.
Violation count log is currently misleading.
errors.size() is the number of grouped properties, not total constraint violations, but the message says “violations”.
Proposed fix
- log.info("Constraint violation: {} violations", errors.size());
+ int violationCount = errors.values().stream().mapToInt(java.util.List::size).sum();
+ log.info("Constraint violation: {} violations", violationCount);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@platform-infra/src/main/java/com/stablecoin/payments/platform/infrastructure/exception/BaseGlobalExceptionHandler.java`
around lines 44 - 50, The log in BaseGlobalExceptionHandler currently logs
errors.size() which is the number of grouped property keys, not the total number
of constraint violations; update the logging to compute the real total (e.g.,
sum the sizes of the lists in the grouped map produced from
ex.getConstraintViolations()) and log that total count (or both the number of
properties and total violations) before returning ApiError.withErrors; refer to
the variables/methods errors, ex.getConstraintViolations(), and
ApiError.withErrors to locate the change.
Comment on lines
+19
to
+20
| public void publish(Object event) { | ||
| var key = resolveKey(event); |
There was a problem hiding this comment.
Add an explicit null guard for event at the API boundary.
Line 19 currently allows null, which later fails with a less-informative NPE in key resolution/logging. Fail fast with a clear message.
Proposed fix
+import java.util.Objects;
import java.util.List;
@@
`@Transactional`(propagation = Propagation.MANDATORY)
public void publish(Object event) {
+ Objects.requireNonNull(event, "event must not be null");
var key = resolveKey(event);
outbox.schedule(event, key);
log.debug("Scheduled outbox event type={} key={}", event.getClass().getSimpleName(), key);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@platform-infra/src/main/java/com/stablecoin/payments/platform/infrastructure/messaging/AbstractOutboxEventPublisher.java`
around lines 19 - 20, Add an explicit null guard at the API boundary inside the
publish method of AbstractOutboxEventPublisher: at the very start of
publish(Object event) call Objects.requireNonNull(event, "event must not be
null") (or throw a clear IllegalArgumentException) so resolveKey(event) and
subsequent logging never see a null and the failure message is descriptive;
ensure you add the necessary import for java.util.Objects if using
requireNonNull.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
stablebridge.service,stablebridge.api-library,stablebridge.client-library) — eliminate ~1,800 lines of duplicated build config across 30 modulesApiErrorrecord replacing 10 identical copiesTestUtils,HexagonalArchitectureRules,TestContainerSupport,TestSecurityConfigreplacing duplicated test infrastructure across 10 servicesAbstractOutboxHandler,AbstractOutboxEventPublisher,BaseGlobalExceptionHandlerreplacing duplicated runtime code across 10 servicesNet impact: +1,189 / -5,114 lines (~3,925 lines eliminated)
Each service
build.gradle.ktsreduced from ~205 lines to 15-39 lines. API modules reduced to 3 lines. Client modules reduced to 7 lines.Test plan
./gradlew test— 131 tasks)./gradlew integrationTest— 10 services)./gradlew businessTest— 10 services)./gradlew compileJava— 39 modules)Closes STA-202
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Refactor
Chores