feat(dedup): PR-PLATFORM-2 — compound-PK processed_kafka_events impl #2
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/pr-platform-2-processed-events"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Replaces v1.0 scaffold stubs in
processed-kafka-events/with the productionimplementation specified in ADR-0014
§D-4 (compound-PK dedup contract) + §L-7 / §L-8 (pg_partman weekly RANGE +
90-day retention) + §L-10 (week_start derived from event timestamp) +
§L-12 (per-pod broadcast dedup via scope suffix).
Public API
ProcessedKafkaEvent@Entity,@EmbeddedIdcomposite PK,@Immutable(append-only — no@Versionrequired).ProcessedKafkaEventId@EmbeddableJava 17record— composite PK(consumer_scope_id, event_id, week_start).ProcessedKafkaEventRepositoryinsertOrNothing(...)(ON CONFLICT DO NOTHING).DedupGuardtryClaim(scope, eventId, eventTime, topic, partition, offset)→boolean. Own@Transactionalboundary.DedupMetricsBinderim2be_dedup_claims_total{consumer_scope_id, outcome}+ gaugeim2be_dedup_active_partitions_total{consumer_scope_id}.DedupAutoConfiguration@ConditionalOnProperty("im2be.dedup.enabled", "true").DedupPropertiesim2be.dedup.{enabled, default-scope-id}with@NotBlankvalidation.Key design decisions
@EmbeddedIdover@IdClass: composite-key record givesequals/hashCode/toString/Serializablefor free — Hibernate 6.6 (SpringBoot 3.5.14) supports records as
@Embeddable.eventIdastext, notUUID: aligns with ADR-0014's mint-agnosticcontract (UUIDv7 for new events, UUIDv3 for deterministic events like
TicketExpired). The dedup table only needs equality.week_startderived from event timestamp, NOT ingestion: stabilisesidempotency under reprocessing (replay a topic from offset 0 a week later
→ same compound keys → same dedup decisions). UTC-normalised — documented
@Transactionalboundary ontryClaim: business-logic failuremust NOT roll back the dedup mark. Operators wanting at-least-once-with-
rollback should NOT use this guard.
@Param-bound positional native query forauditability; the SpEL-on-native-query pattern is fragile and not
needed here.
stays light, consumers supply their compile-scoped versions matching
their Actuator / Kafka starter choices.
Test plan
mvn -B clean install— BUILD SUCCESS, zero[WARNING]lines (rule 62).mvn -B -pl processed-kafka-events test— 20 unit tests green:ProcessedKafkaEventIdTest(5) — recordequals/hashCode/Serializableround-trip.WeekStartDerivationTest(5) — Monday boundary cases incl.UTC-Sunday-vs-CET-Monday.
DedupGuardTest(7) — mocked repo verifies claimed/duplicate outcomes,week_start derivation flows into
insertOrNothing, blank/null argvalidation.
DedupAutoConfigurationTest(3) —ApplicationContextRunnerwith H2:disabled by default → no beans; enabled → all beans present.
mvn -B -pl processed-kafka-events verify— 4 IT tests greenagainst a real PostgreSQL 15 Testcontainer:
true, secondfalse.event_id+ week, different scope → bothtrue.event_id, twodifferent weeks → both
true.README
processed-kafka-events/README.mdcarries the full opt-in template,pg_partman setup SQL (
premake=4, 90d retention, daily pg_cron), sample@KafkaListenerusage, and observability surface.Cross-references
im2be-mono).planning/26-stage-b-outbox-parity.md— Wave PLATFORM rolloutoutbox-publisher(producer side; PR-PLATFORM-1)Replace v1.0 scaffold stubs in processed-kafka-events with the production implementation specified in ADR-0014 §D-4 / §L-7 / §L-8 / §L-10 / §L-12. Entity model - ProcessedKafkaEvent: @Entity, @EmbeddedId composite key, @Immutable (append-only — no @Version required, carved out of EntityVersionParity ArchRule by @Immutable). Indexed on (topic, partition) for forensics. - ProcessedKafkaEventId: @Embeddable record (Hibernate 6.6 supports records as embeddable IDs). Components: consumer_scope_id, event_id (UUID rendered as text), week_start (LocalDate). Serializable. Repository - Atomic insertOrNothing(...) via native query with `ON CONFLICT (consumer_scope_id, event_id, week_start) DO NOTHING`, positional @Param binding (no SpEL) for auditability. Returns 1 on first insert, 0 on conflict — the dedup signal. - countByIdConsumerScopeIdAndIdWeekStart for the active-partitions gauge. - "offset" quoted (PG reserved word). DedupGuard service - tryClaim(scope, eventId, eventTime, topic, partition, offset) → boolean. - Week-start derivation: UTC-normalised, TemporalAdjusters.previousOrSame(MONDAY) — Sunday 22:30 UTC = previous Monday (NOT local-CEST Monday). L-10 stability boundary documented in Javadoc. - Own @Transactional boundary so business-logic failure does NOT roll back the dedup mark. - OTel span `im2be.dedup.try_claim` with attrs scope, event_id, week_start, topic, partition, offset, outcome. Micrometer counter `im2be_dedup_claims_total{consumer_scope_id, outcome}`. Autoconfig + properties - DedupAutoConfiguration: @ConditionalOnProperty("im2be.dedup.enabled"), self-scoped @EntityScan + @EnableJpaRepositories, @ConditionalOnMissingBean fallbacks for Clock + Tracer. Brings spring-boot-starter-validation for @NotBlank. - DedupProperties: im2be.dedup.{enabled,default-scope-id} with @NotBlank. - DedupMetricsBinder: counter + lazy-registered gauge polling countByIdConsumerScopeIdAndIdWeekStart at the current week. Tests (24 total, all green) - ProcessedKafkaEventIdTest (5): record equals/hashCode + Serializable round-trip. - WeekStartDerivationTest (5): UTC-Sunday→prev-Monday boundary; Wednesday → that week; CET-Monday-vs-UTC-Sunday. - DedupGuardTest (7): mocked repo; happy claimed/duplicate paths; week_start derivation flows into insertOrNothing; blank/null arg validation. - DedupAutoConfigurationTest (3): ApplicationContextRunner with H2; disabled by default → no beans; enabled → DedupGuard + Binder + Properties. - DedupGuardIT (4, Testcontainers + PG 15): happy path + scope independence + week-start independence (L-10 boundary) + concurrent race (10 threads, exactly 1 winner). Maven deps added - spring-boot-starter-validation (compile — drives @NotBlank). - spring-kafka, micrometer-core, opentelemetry-api (provided — consumers bring their compile-scoped versions). - Test: testcontainers postgresql + junit-jupiter (1.20.4), flyway-core + flyway-database-postgresql, postgresql JDBC, h2, micrometer simple registry (transitively). - maven-failsafe-plugin executions wired so `mvn verify` triggers the IT. README at processed-kafka-events/README.md covers: purpose, 3-paragraph overview, public API table, opt-in property setup, full pg_partman weekly RANGE + retention + pg_cron SQL template, sample @KafkaListener usage, observability surface, cross-refs to ADR-0014 + .planning/26. Build verification (rule 62): - mvn -B clean install → BUILD SUCCESS - grep -c '^\[WARNING\]' /tmp/dedup-build.log → 0 - mvn -B -pl processed-kafka-events test → 20 unit tests green - mvn -B -pl processed-kafka-events verify → 4 IT tests green against real PostgreSQL 15 TestcontainerShow previous round
hib-pr-reviewer review — PR #2 (affinity-intelligence-rework/im2be-platform-libs)
Round 1 — head
e366f11f0104, basemain, triggersynchronizeTL;DR: NEEDS_WORK — kept 2 agreed major findings, verified 1 unique-B major and 2 unique-A minors; no findings dropped.
Summary
Arbiter reconciliation — round 1
No prior run history in Memora (first arbitration for this PR). All five reconciled findings were verified against HEAD source files before being kept.
Agreed findings kept (2): Both A and B independently flagged the
@Transactionalpropagation defect (exact same file + line) and the deaddefaultScopeIdproperty. These are the strongest signals.Unique-to-B finding verified and kept (1): B's observation that
@Validatedis placed onDedupAutoConfigurationrather than onDedupPropertiesitself is confirmed.DedupProperties.javahas no@Validatedannotation; Spring Boot'sConfigurationPropertiesBindingPostProcessoronly enforces JSR-303 constraints when@Validatedis present on the@ConfigurationPropertiesclass. The Javadoc on line 38 explicitly claims the validation is 'wired inDedupAutoConfiguration' — the mechanism is mis-understood. This is a distinct defect from finding #2 (which is about the property being consumed at all).Unique-to-A findings verified and kept (2):
Instant.now()at DedupGuard.java:159 is confirmed — theClockbean exists and reachesDedupMetricsBinderbut neverDedupGuard. Public setters onProcessedKafkaEventare confirmed; the entity uses field access and is annotated@Immutable, making the setters both misleading and unneeded.No findings were dropped.
Blast Radius
The diff touches 15 files within a single Spring Boot autoconfiguration library module (
processed-kafka-events). The library is consumed by multiple aim2be services via autoconfiguration (im2be.dedup.enabled=true), so the@Transactionalpropagation bug and the dead property silently affect every downstream service that enables the module. Changes are self-contained within the dedup package, but the exported API surface (DedupGuard.tryClaim,DedupPropertiesbinding) amplifies the impact across the platform.BLAST_SCORE: 5/10
Risk Indicators
DedupGuard.tryClaim,ProcessedKafkaEventRepository.insertOrNothingCI status (head
e366f11f0104)Overall: ✗ failure
2 checks: 2 pending
Findings (5)
[MAJOR]
@Transactionaluses default REQUIRED propagation — documented "own transaction" guarantee is brokenprocessed-kafka-events/src/main/java/com/aim2be/platform/dedup/DedupGuard.java:141
Agreed — both A and B independently identified this defect at the same line.
Line 141 reads:
The Javadoc (lines 118–122) and README both guarantee: "the INSERT runs in its own transaction so that a subsequent business-logic failure does NOT roll back the dedup mark." With the default
PROPAGATION_REQUIRED, when the caller already holds an open transaction (any@Transactionalservice method or Kafka listener decorated with@Transactional),tryClaimjoins that outer transaction. A rollback in the caller's business logic rolls back the dedupINSERT, allowing the next redelivery attempt to find no row and re-claim the event — the exact duplicate-delivery failure the library is designed to prevent.Fix: change to
@Transactional(propagation = Propagation.REQUIRES_NEW)so the INSERT commits independently of the caller's transaction context:Note:
REQUIRES_NEWsuspends the caller's transaction for the duration of the INSERT, incurring a nested-connection cost on XA datasources. The existingDedupGuardTestmocks the repository andDedupGuardITruns each call in an undecorated test method — neither catches this regression; a dedicated IT with an outer@Transactionaltest method is recommended to lock in the fix.[MAJOR]
@NotBlank defaultScopeIdis validated but never consumed — dead mandatory config propertyprocessed-kafka-events/src/main/java/com/aim2be/platform/dedup/DedupProperties.java:40
Agreed — both A and B independently identified this defect.
DedupProperties.defaultScopeIdcarries@NotBlank(line 40, value defaults to""). The Javadoc on lines 18–19 asserts it "is read byDedupGuardwhen callers use the no-scope overload." Verified:DedupGuardhas exactly onetryClaimsignature — the 6-argument form requiring an explicitconsumerScopeId. There is no 5-argument no-scope variant.getDefaultScopeId()is called exclusively from the test (DedupAutoConfigurationTest.java:68) — never from production code.DedupAutoConfiguration.dedupGuard()(lines 100–107) does not injectDedupPropertiesintoDedupGuard, so the guard has no path to the property even at runtime.Consequence: every service that sets
im2be.dedup.enabled=truemust also supplyim2be.dedup.default-scope-id(or face aConstraintViolationExceptionif the@NotBlankis ever enforced — see finding 3), yet the value has zero effect on dedup behaviour.Fix options:
DedupGuard, injectDedupPropertiesvia the constructor, wire it fromDedupAutoConfiguration.dedupGuard(…).defaultScopeIdfromDedupPropertiesand all documentation until the overload is implemented. Mark it@Preview/ optional if you want to reserve the field.[MAJOR]
@Validatedis onDedupAutoConfiguration, not onDedupProperties—@NotBlankbinding-time validation is silently unenforcedprocessed-kafka-events/src/main/java/com/aim2be/platform/dedup/DedupProperties.java:23
Unique to reviewer B — verified by inspection.
DedupProperties.javais annotated only with@ConfigurationProperties(prefix = "im2be.dedup")(line 23); there is no@Validatedat the class level. The@Validatedannotation lives onDedupAutoConfiguration(line 43 of that file), which activates Spring AOP method-level validation on@Beanfactory methods — it does NOT triggerConfigurationPropertiesBindingPostProcessor/ConfigurationPropertiesJsr303Validatorvalidation on the bound properties object.The Javadoc on
DedupPropertiesline 38 claims the@NotBlankis "evaluated by the standard Spring Boot@Validatedmachinery wired inDedupAutoConfiguration" — this is incorrect. Without@Validateddirectly on the@ConfigurationPropertiesclass,im2be.dedup.enabled=truewith a missing or blankdefault-scope-idstarts the application silently withdefaultScopeId = "", making the advertised startup guard a no-op.This is a distinct defect from finding #2 (property never consumed). Even once an overload is added (fixing #2), the validation gate must also be fixed or the overload would silently call
tryClaimwith a blank scope.Fix: add
@Validateddirectly toDedupProperties:[MINOR]
Instant.now()bypasses the injectedClock—firstSeenAtis not test-controllableprocessed-kafka-events/src/main/java/com/aim2be/platform/dedup/DedupGuard.java:159
Unique to reviewer A — verified by inspection.
Line 159:
This
nowis passed directly torepository.insertOrNothing(…)as thefirstSeenAtargument.DedupAutoConfigurationregisters a@ConditionalOnMissingBean Clock dedupClock()(returningClock.systemUTC()) precisely so test and staging contexts can inject a fixed clock.DedupMetricsBindercorrectly receives this clock via its constructor. HoweverDedupGuard's constructor accepts only(repository, metrics, tracer)— the clock is never wired through.Fix: add
ClocktoDedupGuard's constructor, inject fromDedupAutoConfiguration.dedupGuard(…), and replace the direct call:This lets
DedupGuardTestpinfirstSeenAtin assertions without racing against wall-clock time.[MINOR] Public setters on a Hibernate
@Immutablefield-access entity are misleading and unneededprocessed-kafka-events/src/main/java/com/aim2be/platform/dedup/ProcessedKafkaEvent.java:125
Unique to reviewer A — verified by inspection.
ProcessedKafkaEventis annotated@Immutable(line 55) and uses field access (the@EmbeddedIdannotation is on theidfield at line 59-60, not on a getter). Hibernate populates fields directly at hydration time and never dirty-checks or flushes updates on@Immutableentities.Lines 125–158 expose
publicsetters forid,firstSeenAt,topic,partition, andoffset. These are:@Immutable— any caller can mutate an entity instance that is semantically append-only post-insert.Fix: narrow all setter visibility to
package-private(drop thepublicmodifier). If external test builders need construction, promote the existingProcessedKafkaEvent(id, topic, partition, offset)convenience constructor and rely on that instead of setters.Verdict
NEEDS_WORK
hib-pr-reviewer • round 1 • 5 findings (3M/2m) • 2026-05-26T22:47:17.667Z → 2026-05-26T22:50:40.148Z • posted-as: pr-reviewer-bot
Show previous round
hib-pr-reviewer review — PR #2 (affinity-intelligence-rework/im2be-platform-libs)
Round 2 — head
295aeaabac56, basemain, triggersynchronizeTL;DR: NEEDS_WORK — kept 2 findings (1 major agreed by both reviewers, 1 minor unique-to-B verified); both confirmed by direct file reads at HEAD.
Summary
Round 2 Arbitration — reconciliation summary
Recalled Memora context: no prior run history found (new store). Memora persistence attempted 4 times; all failed with
Tag 'memora/issues' is not in the allowed tag list(auto-classifier is assigning a tag not in this instance's allowlist — not a content or logic error on my part).Verification performed:
DedupGuard.javain full. Confirmed line 236–247: the 5-arg no-scope overload carries no@Transactionalannotation; line 247 is a barereturn tryClaim(defaultScopeId, …)self-call that cannot route through the Spring CGLIB proxy. The 6-arg overload at line 160 carries@Transactional(propagation = Propagation.REQUIRES_NEW)— but that annotation is unreachable via the self-call. Major finding confirmed at stated line.DedupAutoConfiguration.javain full. Confirmed line 67:return GlobalOpenTelemetry.getTracer("com.aim2be.platform.dedup")— evaluated eagerly at bean-creation time with no@AutoConfigureAfterordering guard and noOpenTelemetrybean injection. Minor finding confirmed at stated line.Reconciliation decisions:
Total kept: 2 (1 major, 1 minor). No findings dropped.
Blast Radius
The diff is contained within the
processed-kafka-eventsmodule (15 files), butDedupGuard.tryClaimis the single public dedup API surface that every consuming@KafkaListenercalls. The no-scope overload is the usage pattern shown in the README and is likely the most common entry point across all consumers. A broken transactional guarantee at this layer propagates silently to every service in the platform that adopts the library.BLAST_SCORE: 6/10
Risk Indicators
DedupGuard.tryClaim,ProcessedKafkaEventRepository.insertOrNothingCI status (head
295aeaabac56)Overall: ✗ failure
2 checks: 2 pending
Findings (2)
[MAJOR] No-scope
tryClaimoverload bypasses@Transactional(REQUIRES_NEW)via self-invocation — dedup isolation guarantee silently broken for the primary consumer APIprocessed-kafka-events/src/main/java/com/aim2be/platform/dedup/DedupGuard.java:247
The 5-arg convenience overload (line 236) carries no
@Transactionalannotation. Its sole delegation —return tryClaim(defaultScopeId, eventId, eventTime, topic, partition, offset)at line 247 — is athis.-call that routes to the raw target object, not the Spring CGLIB proxy. The@Transactional(propagation = Propagation.REQUIRES_NEW)declared on the 6-arg method at line 160 is therefore never triggered when a caller enters through the no-scope path.Impact: a
@KafkaListenermethod that already runs inside a transaction (the common case) and calls the no-scope overload will execute the dedup INSERT inside the caller's ambient transaction. If a downstream business-logic failure rolls that transaction back, the dedup mark is rolled back too — allowing the same event to be reprocessed on the next retry. The README and class Javadoc explicitly guarantee the opposite ("a subsequent business-logic failure does NOT roll back the dedup mark").Fix: annotate the no-scope overload with
@Transactional(propagation = Propagation.REQUIRES_NEW):Spring intercepts the outer call via proxy, suspending any ambient transaction and starting a fresh
REQUIRES_NEWone. The inner self-call to the 6-arg method then participates in that already-active transaction; the 6-arg method's own annotation is bypassed by self-invocation but the surrounding transaction already satisfies the contract.Test gap:
DedupGuardITdoes not cover the scenario of calling the no-scope overload from within an ambient transaction and verifying that an outer rollback does NOT roll back the dedup mark. This case should be added alongside the fix.[MINOR]
dedupTracer()snapshotsGlobalOpenTelemetryeagerly at bean-creation time — permanently noop tracer when OTel SDK autoconfiguration runs afterDedupAutoConfigurationprocessed-kafka-events/src/main/java/com/aim2be/platform/dedup/DedupAutoConfiguration.java:67
Line 67:
return GlobalOpenTelemetry.getTracer("com.aim2be.platform.dedup");is evaluated once at Spring context-refresh time.GlobalOpenTelemetry.getTracer()captures theOpenTelemetrySDK that is globally registered at that moment — it does not produce a lazy-delegating proxy. If the consuming service's OTel SDK autoconfiguration (e.g.opentelemetry-spring-boot-starter'sOpenTelemetryAutoConfiguration) runs afterDedupAutoConfiguration— which is possible because no@AutoConfigureAfterordering constraint is declared on the class —GlobalOpenTelemetry.get()still holds the noop SDK at call time, anddedupTracer()returns a permanently-noopTracer. Allim2be.dedup.try_claimspans are silently dropped with no runtime error.Scope: this only bites the programmatic-SDK path. When the OTel Java Agent is used, the agent pre-wires
GlobalOpenTelemetrybefore any Spring beans are created, so ordering is irrelevant.Fix: accept an optional
OpenTelemetrybean parameter and fall back to the global only when no bean is present:Spring DI guarantees the injected
OpenTelemetrybean is fully initialised before this factory method fires; no@AutoConfigureAfteron an optional dependency is needed.Verdict
NEEDS_WORK
hib-pr-reviewer • round 2 • 2 findings (1M/1m) • 2026-05-26T23:01:15.166Z → 2026-05-26T23:03:20.287Z • posted-as: pr-reviewer-bot
Show previous round
hib-pr-reviewer review — PR #2 (affinity-intelligence-rework/im2be-platform-libs)
Round 3 — head
bbdcd13c363d, basemain, triggersynchronizeTL;DR: CONDITIONAL_APPROVE — kept 3 unique-to-B findings (all verified at HEAD): eager OTel snapshot with incorrect Javadoc, missing no-scope overload tests with stale comment, and stale README @NotBlank claim; all prior R1/R2 findings confirmed resolved.
Summary
Reviewer A's JSON was truncated and returned
[]formally (0 findings), though prose echoed the same OTel eager-resolution issue as B's Finding 1. All prior R1/R2 findings confirmed resolved by both reviewers. All three B findings were verified against HEAD:DedupAutoConfiguration.java:77-79 —
otelProvider.getIfAvailable(OpenTelemetry::noop).getTracer(...)executes synchronously inside the@Beanmethod body; theTraceris a concrete snapshot taken atApplicationContext.refresh()time. The Javadoc at lines 64-65 incorrectly states the lookup is "deferred until this bean's first method invocation." No@AutoConfigureAfteris present. Finding confirmed — kept as MINOR.DedupGuardTest.java:56-57 — Comment reads
// own test below (DedupGuardNoScopeOverloadTest).; aGlobacross the entire checkout found zero files matchingDedupGuardNoScopeOverloadTest*. The no-scopetryClaimoverload (which carries the R2F1 self-invocation fix) has zero unit or integration test coverage. Finding confirmed — kept as MINOR.README.md:52 — Row states
@NotBlank validation on default-scope-id when enabled;DedupProperties.javahas no jakarta constraint ondefaultScopeId(the field defaults to""and is intentionally optional). Finding confirmed — kept as INFO.Memora persistence attempted but failed due to system-level tag allowlist rejection; run summary not stored.
Blast Radius
This module is a Spring Boot autoconfiguration library exported for consumption by multiple aim2be services. The
DedupAutoConfigurationwires beans shared across all consumers, andDedupGuardis the single dedup API entry point. Incorrect OTel bean ordering affects every consuming service's tracing fidelity at startup; the missing test gap leaves a behaviorally-critical overload unvalidated before it ships to all consumers.BLAST_SCORE: 5/10
Risk Indicators
dedupTracer,insertOrNothing,tryClaimInternalCI status (head
bbdcd13c363d)Overall: ✗ failure
2 checks: 2 pending
Findings (3)
[MINOR]
ObjectProvider.getIfAvailable()is called eagerly at bean-creation time; Javadoc incorrectly claims deferred resolution;@AutoConfigureAfteris absentprocessed-kafka-events/src/main/java/com/aim2be/platform/dedup/DedupAutoConfiguration.java:78
otelProvider.getIfAvailable(OpenTelemetry::noop).getTracer("com.aim2be.platform.dedup")at line 78 executes synchronously inside the@Beanfactory method, which runs duringApplicationContext.refresh(). The resultingTraceris a concrete snapshot taken at bean-creation time — if the OTel SDK auto-configuration'sOpenTelemetrybean has not yet been registered at that moment,getIfAvailable()returns the noop fallback permanently.The Javadoc at lines 64–65 states: "ObjectProvider is used so the lookup is deferred until this bean's first method invocation" — this is factually wrong. The
ObjectProvideris resolved immediately, not lazily.No
@AutoConfigureAfteris present onDedupAutoConfiguration, so initialization order relative to any OTel SDK starter is undefined.Recommended fix (Option A — no hot-path overhead): add a class-level ordering hint:
Using
name(not the class literal) avoids a hard compile dependency on the OTel instrumentation starter. Also correct the Javadoc to drop the incorrect "deferred" claim.Option B (true lazy resolution): inject
ObjectProvider<OpenTelemetry>intoDedupGuarditself and callotelProvider.getIfAvailable(OpenTelemetry::noop).getTracer(...)on eachspanBuilderinvocation — resilient to any ordering scenario but adds a provider lookup pertryClaimcall.[MINOR] Stale comment references non-existent
DedupGuardNoScopeOverloadTest; no-scopetryClaimoverload has zero test coverageprocessed-kafka-events/src/test/java/com/aim2be/platform/dedup/DedupGuardTest.java:57
Lines 56–57 read:
DedupGuardNoScopeOverloadTestdoes not exist anywhere in the repository (confirmed via Glob across the full checkout). The no-scopetryClaim(String, Instant, String, int, long)overload — which carries the R2F1 fix by callingtryClaimInternalinstead of the public 6-arg overload — is entirely untested in both unit and integration tests.Proposed fix: add a
DedupGuardNoScopeOverloadTest(or extendDedupGuardTest) covering at minimum:tryClaimwith a configured default scope delegates toinsertOrNothingwith that scope and returns the repository result correctly.tryClaimwithnull(or blank)defaultScopeIdthrowsIllegalStateExceptionwith a message mentioningim2be.dedup.default-scope-id.[INFO] README public-API table incorrectly claims
@NotBlankvalidation ondefault-scope-idprocessed-kafka-events/README.md:52
README.md line 52 states:
DedupProperties.javahas no@NotBlank(or any jakarta constraint) ondefaultScopeId— the field defaults to""and is intentionally optional (the R1 fix removed the over-constraint). The README still advertises the removed constraint, misleading consumers into believing a misconfigured blankdefault-scope-idis caught at startup rather than at the first no-scopetryClaimcall.Proposed fix: update line 52 to reflect the actual behaviour:
Verdict
CONDITIONAL_APPROVE
hib-pr-reviewer • round 3 • 3 findings (2m/1i) • 2026-05-26T23:12:16.550Z → 2026-05-26T23:14:28.199Z • posted-as: pr-reviewer-bot
Show previous round
hib-pr-reviewer review — PR #2 (affinity-intelligence-rework/im2be-platform-libs)
Round 4 — head
c28328d2dc48, basemain, triggersynchronizeTL;DR: CONDITIONAL_APPROVE — kept 2 minor findings (1 agreed A+B: stale GlobalOpenTelemetry Javadoc with broken @link; 1 unique-to-A verified: README mislabels DedupGuard as @Service); all 10 prior-round findings confirmed resolved.
Summary
Round 4 Arbitration
Prior findings (R1–R3, 10 total): Both A and B independently confirm all ten are resolved. Not re-raised.
Agreed finding (A+B): Both reviewers flag
DedupAutoConfiguration.javaJavadoc using{@link GlobalOpenTelemetry}as the OTel fallback. A cites line 27, B cites line 26.Readconfirms the text lives at line 26.GrepconfirmsGlobalOpenTelemetryhas zero import statements in the file — onlyio.opentelemetry.api.OpenTelemetryandio.opentelemetry.api.trace.Tracerare imported. The actual runtime fallback (line 111) isotelProvider.getIfAvailable(OpenTelemetry::noop). B's body is richer (broken@linkreference + Java-Agent span-loss implication) and is used as the canonical text. Kept as minor.Unique-to-A finding: README.md line 49 labels
DedupGuardas@Service.GreponDedupGuard.javafor@Service|@Componentreturned no matches — the bean is registered exclusively through the@Beanfactory inDedupAutoConfiguration. Finding verified. Kept as minor.Final tally: A=2, B=1 → kept=2, dropped=0. Memora persistence attempted; server-side tag allowlist blocked creation (auto-assigned tag
memora/issuesis not in allowlist).Blast Radius
Both findings are documentation-only defects in a new, self-contained autoconfiguration module. The GlobalOpenTelemetry Javadoc error has a latent behavioral implication (lost OTel spans for Java-Agent deployments) but requires no schema or runtime change to fix. The README label error is cosmetic. Neither touches shared infrastructure, exported contracts, or migration files.
BLAST_SCORE: 2/10
CI status (head
c28328d2dc48)Overall: ✗ failure
2 checks: 2 pending
Findings (2)
[MINOR] Javadoc claims
GlobalOpenTelemetryfallback but code usesOpenTelemetry::noop— broken@link+ silent trace loss for Java-Agent deploymentsprocessed-kafka-events/src/main/java/com/aim2be/platform/dedup/DedupAutoConfiguration.java:26
Line 26 reads:
GlobalOpenTelemetryis neither imported in this file (confirmed: no import for it; onlyio.opentelemetry.api.OpenTelemetryandio.opentelemetry.api.trace.Tracerare imported) nor referenced in the implementation. The actual deferred supplier at line 111 is:Two correctness problems:
{@link GlobalOpenTelemetry}is an unresolvable Javadoc reference — will emit a Javadoc warning/broken link.GlobalOpenTelemetry, not as a Spring bean) will getOpenTelemetry.noop()fromgetIfAvailable(), silently discarding allim2be.dedup.try_claimspans.Fix option A (accurate noop documentation — simplest):
Fix option B (restore Java-Agent compatibility in the lambda):
Option B is safe inside the deferred lambda (the R2 bug was the eager snapshot, not
GlobalOpenTelemetryper se) and should be preferred if any aim2be service uses the Java Agent.[MINOR] README public-API table incorrectly labels
DedupGuardas@Serviceprocessed-kafka-events/README.md:49
Line 49 in the public-API table reads:
DedupGuardcarries no@Service(or@Component) annotation — confirmed byGreponDedupGuard.java(zero matches for@Service|@Component). The bean is registered exclusively through the@Beanfactory method inDedupAutoConfiguration. This distinction matters: consumers scanning for@Servicebeans to exclude or decorate will not findDedupGuard.Fix:
Verdict
CONDITIONAL_APPROVE
hib-pr-reviewer • round 4 • 2 findings (2m) • 2026-05-26T23:25:47.716Z → 2026-05-26T23:27:48.043Z • posted-as: pr-reviewer-bot
Show previous round
hib-pr-reviewer review — PR #2 (affinity-intelligence-rework/im2be-platform-libs)
Round 5 — head
364bee6378c6, basemain, triggersynchronizeTL;DR: CONDITIONAL_APPROVE — kept 2 unique-to-one minor findings (both verified via Read); no blocking issues; all 12 prior-round findings confirmed resolved.
Summary
Arbiter reconciliation — Round 5
Both reviewers confirmed all 12 prior-round findings (R1–R4) are resolved. Each submitted exactly one new, unique-to-one minor finding.
Finding A (unique to A) —
DedupAutoConfiguration.java:123dead null guard onGlobalOpenTelemetry.get(): verified viaReadat lines 121–125. Code readsreturn global != null ? global : OpenTelemetry.noop();exactly as cited. The OTel Java API specifiesGlobalOpenTelemetry.get()never returnsnull; the else branch is unreachable. Kept — severity minor.Finding B (unique to B) —
DedupGuardIT.java:56floatingpostgres:15-alpinetag: verified viaReadat line 56.new PostgreSQLContainer<>("postgres:15-alpine")confirmed exactly as cited; floating mutable tag can silently shift under CI. Kept — severity minor.No agreed findings. No findings dropped. Memora persistence attempted; tool permissions not available in this session (fallback noted in arbiter log).
Blast Radius
The diff introduces a new Spring Boot autoconfiguration library module (
processed-kafka-events) exported to all consuming services in the mono-repo. Changes span 15 files across production beans, JPA entities, a custom repository, Micrometer metrics, OTel tracing, test infrastructure, and README. Any misconfiguration in the autoconfiguration or entity model propagates to every consumer that opts in viaim2be.dedup.enabled=true.BLAST_SCORE: 5/10
Risk Indicators
DedupGuard.tryClaim,DedupGuard.tryClaimInternal,ProcessedKafkaEventRepository.insertOrNothing,DedupAutoConfiguration.dedupGuardCI status (head
364bee6378c6)Overall: ✗ failure
2 checks: 2 pending
Findings (2)
[MINOR]
GlobalOpenTelemetry.get()never returns null — null guard is dead codeprocessed-kafka-events/src/main/java/com/aim2be/platform/dedup/DedupAutoConfiguration.java:123
Lines 121–125 read:
GlobalOpenTelemetry.get()is specified by the OTel Java API (io.opentelemetry:opentelemetry-api) to never returnnull— when no SDK or Java Agent has registered a global instance, the method returns its ownOpenTelemetry.noop()singleton. The ternary'selsebranch is therefore unreachable and misleads readers into thinking the API can return null.Simplify the factory to:
This is shorter, self-documents the three-tier intent (Spring-bean →
GlobalOpenTelemetry.get()→ noop via OTel's own fallback), and removes the misleading null check. Update the comment at line 117 accordingly:→ noop (GlobalOpenTelemetry.get() returns its own noop when no SDK/Agent is registered).[MINOR] Unpinned mutable Docker image tag
postgres:15-alpineprocessed-kafka-events/src/test/java/com/aim2be/platform/dedup/DedupGuardIT.java:56
postgres:15-alpineis a floating tag: Docker Hub silently re-points it whenever a new 15.x patch ships. If the tag moves mid-sprint, CI may start failing (or, worse, pass on changed Postgres behaviour) without any change in this repository.Proposed fix — pin to the specific patch that was verified when writing the IT:
or, for stricter reproducibility, annotate with a digest comment:
This matches the Testcontainers community best-practice of immutable image references in test infrastructure.
Verdict
CONDITIONAL_APPROVE
hib-pr-reviewer • round 5 • 2 findings (2m) • 2026-05-26T23:35:28.319Z → 2026-05-26T23:36:43.807Z • posted-as: pr-reviewer-bot
Show previous round
hib-pr-reviewer review — PR #2 (affinity-intelligence-rework/im2be-platform-libs)
Round 6 — head
043413236768, basemain, triggersynchronizeTL;DR: CONDITIONAL_APPROVE — kept 2 minor unique-to-one findings (both code-verified); all 14 prior-round findings confirmed resolved.
Summary
Arbiter Reconciliation — Round 6
Memora context: No prior run history found (first time arbiter is invoked with Memora for this PR); Memora persistence calls failed due to tag allowlist/permission errors — noted but non-blocking for the arbitration.
Prior findings (R1–R5): Both reviewers independently confirmed all 14 prior-round findings are resolved. No prior findings re-raised.
New findings — A=1, B=1, both unique-to-one; both verified against HEAD.
Finding A —
@Modifying insertOrNothinglacks@TransactionalRead
ProcessedKafkaEventRepository.javalines 47–64: confirmed@Modifying+@Querywith no@Transactionalon the method.DedupGuard.javagrep confirmed the production call at line 220 is always inside@Transactional(REQUIRES_NEW)— so the current path is safe. However any direct caller outside a write-transaction context (test utility, migration script, future caller) will inheritSimpleJpaRepository's class-level@Transactional(readOnly = true)and fail at the PostgreSQL driver level. Kept.Finding B — No
serialVersionUIDonProcessedKafkaEventIdRead
ProcessedKafkaEventId.javalines 43–52: confirmedimplements Serializablewith noserialVersionUIDfield. Record body closes at line 52 with no static SVUID. The risk of SVUID drift on component rename (e.g. L2 cache, Infinispan) is real. Kept.Outcome: 2 minor findings kept, 0 dropped, no blocking issues.
Blast Radius
All 15 changed files are confined to the
processed-kafka-eventssubmodule. The module is a library distributed to consuming services, so its exported surface (DedupGuard, ProcessedKafkaEventRepository) is used beyond this repo, but the blast radius of the two remaining minor issues is low — neither affects the primary call path.BLAST_SCORE: 3/10
Risk Indicators
insertOrNothing,tryClaimInternal,tryClaimCI status (head
043413236768)Overall: ✗ failure
2 checks: 2 pending
Findings (2)
[MINOR]
@ModifyingoninsertOrNothinghas no@Transactional— direct callers inherit read-only semanticsprocessed-kafka-events/src/main/java/com/aim2be/platform/dedup/ProcessedKafkaEventRepository.java:47
Lines 47–64 declare
@Modifying+@Querywith no@Transactionalon the method. Spring Data JPA'sSimpleJpaRepositorycarries a class-level@Transactional(readOnly = true); custom query methods without their own annotation inherit it.Current production path is safe:
DedupGuard.tryClaimInternal(called from both public overloads at lines 166 and 264 ofDedupGuard.java, each annotated@Transactional(REQUIRES_NEW)) always provides a write transaction thatinsertOrNothingjoins.Risk for future callers: Any direct call to
repository.insertOrNothing(...)made outside a write transaction — e.g., a test utility, an ad-hoc migration script, or a future service method — will execute insidereadOnly = true. PostgreSQL rejects this at the driver level:ERROR: cannot execute INSERT in a read-only transaction.Fix: add
@Transactional(defaultREQUIREDpropagation) to the method declaration:When called from the existing
REQUIRES_NEWcontext the transaction join is idempotent; when called standalone a proper write transaction is created automatically.[MINOR] No
serialVersionUIDonSerializablerecord used as JPA@EmbeddedIdprocessed-kafka-events/src/main/java/com/aim2be/platform/dedup/ProcessedKafkaEventId.java:51
Line 51:
LocalDate weekStart) implements Serializable {— the record implementsSerializablebut declares noserialVersionUID. Java computes the SVUID from the class signature (component names + types).If any component is later renamed (e.g.
consumerScopeId→scopeId), the auto-computed SVUID silently changes. Any previously serialized instances — e.g. from a Hibernate L2 cache backed by Java serialization (EhCache, Infinispan), or a distributed session store — would throwjava.io.InvalidClassExceptionon deserialization.ProcessedKafkaEventIdTest.serialization_roundTripsCleanly()validates the current round-trip but cannot catch SVUID drift across refactors.Fix — records permit static fields:
This pins the contract at v1 — any future breaking structural change should increment it.
Verdict
CONDITIONAL_APPROVE
hib-pr-reviewer • round 6 • 2 findings (2m) • 2026-05-26T23:52:48.400Z → 2026-05-26T23:55:03.791Z • posted-as: pr-reviewer-bot
Show previous round
hib-pr-reviewer review — PR #2 (affinity-intelligence-rework/im2be-platform-libs)
Round 7 — head
114ae2080225, basemain, triggersynchronizeTL;DR: CONDITIONAL_APPROVE — kept 1 unique-to-A minor finding (verified at HEAD); B had 0 findings; all 16 prior-round issues confirmed resolved.
Summary
Arbitration — Round 7
Memora recall: No prior run history found in store (create-new path attempted; MCP tag-allowlist error prevented persistence — server auto-assigns
memora/issueswhich is not in the allowlist). Proceeding to arbitration.Finding disposition:
Verification of unique-to-A finding: Read
DedupMetricsBinder.javaat HEAD. Line 104-109 confirms:The two-argument
Gauge.builder(String, Supplier<Number>)overload makes the supplier the state object, held byDefaultGaugeviaWeakReference. TheactivePartitionGaugesmap stores theGaugeinstance, not the supplier — so the lambda() -> currentWeekCount(scope)has no surviving strong reference and is eligible for GC. Post-collection the gauge silently returnsNaN. Reviewer A's fix (use the three-argument form withthisas the strongly-reachable state object) is the canonical Micrometer pattern for long-lived gauges. Finding is grounded and kept.Reviewer B returned zero findings — divergence is genuine, not a formatting artifact.
Prior-round closures: All 16 findings from rounds 1–6 were confirmed resolved by the round-7 reviewers and are not re-raised here.
Blast Radius
The affected code is confined to
DedupMetricsBinder, a single metrics-binding bean within theprocessed-kafka-eventsmodule. The bug silently degrades observability (gauge reads NaN) but does not affect core dedup correctness, the compound-PK claim logic, or any downstream consumer. Impact is limited to Prometheus/Grafana dashboards relying onim2be_dedup_active_partitions_total.BLAST_SCORE: 3/10
Risk Indicators
CI status (head
114ae2080225)Overall: ✗ failure
2 checks: 2 pending
Findings (1)
[MINOR] Gauge supplier lambda held only via WeakReference — silently emits NaN after GC
processed-kafka-events/src/main/java/com/aim2be/platform/dedup/DedupMetricsBinder.java:105
Micrometer's
Gauge.builder(String, Supplier<Number>)two-argument overload makes the supplier the state object (T), held byDefaultGaugevianew WeakReference<>(supplier). TheactivePartitionGaugesmap stores the returnedGaugeinstance, not the supplier — so the lambda() -> currentWeekCount(scope)has no other strong reference. After any GC cycle the weak reference is cleared andDefaultGauge.value()returnsDouble.NaNsilently.Fix — use the three-argument form that puts
this(the long-lived Spring bean) as the weakly-referenced state object; the supplier is then a strongly-heldToDoubleFunction:The bean singleton is reachable for the application's lifetime, so the gauge will never spuriously return
NaN. Thescopestring is captured strongly by the state-function lambda (held by theDefaultGauge).Verdict
CONDITIONAL_APPROVE
hib-pr-reviewer • round 7 • 1 finding (1m) • 2026-05-27T00:02:27.782Z → 2026-05-27T00:03:54.642Z • posted-as: pr-reviewer-bot
Show previous round
hib-pr-reviewer review — PR #2 (affinity-intelligence-rework/im2be-platform-libs)
Round 8 — head
dc557d750900, basemain, triggersynchronizeTL;DR: CONDITIONAL_APPROVE — 1 minor finding kept this round.
Summary
Arbiter reconciled 1 (A) + 0 (B) → 1 findings.
CI status (head
dc557d750900)Overall: ✗ failure
2 checks: 2 pending
Findings (1)
[MINOR] No-scope
tryClaimJavadoc{@link}is stale — points to public explicit-scope overload, but implementation calls privatetryClaimInternalprocessed-kafka-events/src/main/java/com/aim2be/platform/dedup/DedupGuard.java:246
Lines 246–247:
The
{@link}resolves to the public 6-argtryClaimoverload. But line 281:…and the accompanying inline comment (lines 276–280) explicitly confirms the method calls
tryClaimInternal, NOT the public overload, specifically to avoid self-invocation proxy bypass (@TransactionalR2 fix).A reader following the
{@link}will believe a public→public delegation occurs and may incorrectly conclude the R2 proxy-bypass regression is still present. ThetryClaimInternalJavadoc at lines 177–184 further contradicts the stale link.Proposed fix — first paragraph of the no-scope overload Javadoc:
Verdict
CONDITIONAL_APPROVE
hib-pr-reviewer • round 8 • 1 finding (1m) • 2026-05-27T00:14:15.611Z → 2026-05-27T00:15:42.846Z • posted-as: pr-reviewer-bot • [bookkeeping fallback]
hib-pr-reviewer review — PR #2 (affinity-intelligence-rework/im2be-platform-libs)
Round 9 — head
7058c52cd7e5, basemain, triggersynchronizeTL;DR: CONDITIONAL_APPROVE — kept 1 verified INFO finding (README test count 20 vs actual 23); all 18 prior R1–R8 findings confirmed resolved, no blocking issues.
Summary
Arbitration Round 9
Reconciliation actions:
Read(README line 188 confirmed:unit tests (4 classes, 20 tests)) andGrep(actual@Testcounts per file).NoScopeOverloadTestclass inDedupGuardTest.javauses fully-qualified@org.junit.jupiter.api.Testannotations (not just@Test), so a simple@Testgrep returns only 7 outer tests. With the 3 nested tests included, the true unit-test count is 23 — confirming the README undercount of 3. Finding kept.Kept: 1 (INFO). Dropped: 0.
Blast Radius
The only actionable finding is a documentation string in README.md. The diff itself touches a single new submodule (processed-kafka-events) with no cross-module API surface changes. Blast radius is minimal — limited to consumers of this library's documentation.
BLAST_SCORE: 1/10
Risk Indicators
CI status (head
7058c52cd7e5)Overall: ✗ failure
2 checks: 2 pending
Findings (1)
[INFO] README unit-test count is off by 3 (says 20, actual 23)
processed-kafka-events/README.md:188
Line 188:
mvn … test # unit tests (4 classes, 20 tests)under-counts by 3.Verified test counts at HEAD:
DedupGuardTestouter class: 7@Testmethods (lines 113–215)DedupGuardTest.NoScopeOverloadTestnested class: 3@org.junit.jupiter.api.Testmethods (lines 71, 84, 96) — these use the fully-qualified annotation and are missed by a plain@Testgrep, which is why the discrepancy is easy to overlook.DedupAutoConfigurationTest: 3@TestmethodsProcessedKafkaEventIdTest: 5@TestmethodsWeekStartDerivationTest: 5@TestmethodsTotal = 23. Change
20 tests→23 teststo keep the quick-start accurate.Verdict
CONDITIONAL_APPROVE
hib-pr-reviewer • round 9 • 1 finding (1i) • 2026-05-27T00:21:56.093Z → 2026-05-27T00:24:08.337Z • posted-as: pr-reviewer-bot