affinity-intelligence-rework/im2be-platform-libs
2
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

feat(outbox): carry W3C traceparent through the outbox to Kafka headers (GC-23 (b) producer half) #24

Merged
hibryda merged 4 commits from feat/outbox-traceparent-carry into main 2026-06-01 16:22:32 +02:00
Conversation 5 Commits 4 Files changed 11 +792 -52
hibryda commented 2026-06-01 16:01:31 +02:00
Owner
Copy link

Header

im2be-platform-libs · GC-23 (b) producer→relay origin (producer half) · branch feat/outbox-traceparent-carry → main

TL;DR

The transactional outbox now carries the producing span's W3C traceparent through to Kafka — captured at outbox-write time, persisted on the row, and re-emitted as Kafka message headers by both the hot relay and the cold poller — so a business event continues the SAME trace_id from the producing service → Kafka → the consuming relay (which extracts it, realtime #8) → Centrifugo → client.

Summary

Closes the producer half of GC-23 boundary (b). The consumer half shipped in im2be-realtime-service #8 (relay propagation.extract of the Kafka-header traceparent); the relay→Centrifugo segment (boundary (a)) is already closed end-to-end (Tempo d2b8b4ab…). This PR makes the outbox the trace bridge across its store-and-forward boundary:

  • New OutboxTraceContext (package-private) — W3C-explicit (W3CTraceContextPropagator, not the configured propagator, so the traceparent header the polyglot consumer expects is always emitted, ADR-0013): captureCurrent(), restore(traceParent, traceState), currentKafkaHeaders(), kafkaHeadersFor(traceParent, traceState).
  • OutboxRecord — trace_parent (VARCHAR 64) + trace_state (VARCHAR 512) columns + accessors (setters truncate to bound). equals/hashCode unchanged (PK-only).
  • OutboxPublisher.publish(...) — captures the current W3C context at write (while the business span is current) and persists it on the row; the AFTER_COMMIT hot relay nests its im2be.outbox.publish.hot span under the restored business context and emits that span's context as Kafka headers via ProducerRecord.
  • OutboxPollerWorker.tryColdPublish(...) — re-emits the persisted traceparent/tracestate verbatim as Kafka headers (the cold poller may run after a restart with no live context), via ProducerRecord.

No-op when no recording span is active (OpenTelemetry.noop() / sampled-out): trace_parent is NULL, no header is emitted, downstream roots its own trace — safe with or without a real OTel SDK, and forward-compatible with services not yet upgraded.

Risk indicators

indicator value
Shared starter (consumers) 7 Java services consume outbox-publisher
Schema change +2 nullable columns (trace_parent, trace_state); local ddl-auto auto-applies; dev/stage/prod owners add a Flyway ALTER TABLE (documented in README)
Kafka send shape send(topic,key,payload) → send(ProducerRecord) in both relays (tests updated for the matcher ripple)
New prod deps none (W3CTraceContextPropagator is in the already-present opentelemetry-api; ProducerRecord/Header via the present spring-kafka/kafka-clients)

Verification

  • mvn -pl outbox-publisher test — green, zero [WARNING] (parent pom failOnWarning=true).
  • Unit tests assert: capture-at-write persists trace_parent on the row; the hot relay's ProducerRecord carries a traceparent header under an active SDK span; the cold poller re-emits a persisted traceparent verbatim (and emits none when null); accessor round-trip + truncation.
  • Deployed end-to-end (business op → client one trace_id) is gated on publishing the new 1.1.0-SNAPSHOT to the Forgejo Maven registry + a family-service --no-cache rebuild (ci.yml does not yet auto-deploy — PR-PLATFORM-CI-1); tracked as the remaining verification step.

Verdict

NEEDS_REVIEW — shared-library change with a send-shape + schema ripple; backward/forward compatible; no new deps. README updated under "Distributed-trace carry through the outbox (GC-23 (b))".

Footer

im2be-platform-libs · GC-23(b) producer half · R0 (open) · outbox-publisher tests green/0-warn · 2026-06-01

## Header **im2be-platform-libs** · GC-23 (b) producer→relay origin (producer half) · branch `feat/outbox-traceparent-carry` → `main` ## TL;DR The transactional outbox now **carries the producing span's W3C `traceparent`** through to Kafka — captured at outbox-write time, persisted on the row, and re-emitted as Kafka message headers by both the hot relay and the cold poller — so a business event continues the SAME `trace_id` from the producing service → Kafka → the consuming relay (which extracts it, realtime #8) → Centrifugo → client. ## Summary Closes the producer half of GC-23 boundary (b). The consumer half shipped in `im2be-realtime-service` #8 (relay `propagation.extract` of the Kafka-header `traceparent`); the relay→Centrifugo segment (boundary (a)) is already closed end-to-end (Tempo `d2b8b4ab…`). This PR makes the outbox the trace bridge across its store-and-forward boundary: - **New `OutboxTraceContext`** (package-private) — W3C-explicit (`W3CTraceContextPropagator`, not the configured propagator, so the `traceparent` header the polyglot consumer expects is always emitted, ADR-0013): `captureCurrent()`, `restore(traceParent, traceState)`, `currentKafkaHeaders()`, `kafkaHeadersFor(traceParent, traceState)`. - **`OutboxRecord`** — `trace_parent` (VARCHAR 64) + `trace_state` (VARCHAR 512) columns + accessors (setters truncate to bound). equals/hashCode unchanged (PK-only). - **`OutboxPublisher.publish(...)`** — captures the current W3C context at write (while the business span is current) and persists it on the row; the AFTER_COMMIT hot relay nests its `im2be.outbox.publish.hot` span under the restored business context and emits that span's context as Kafka headers via `ProducerRecord`. - **`OutboxPollerWorker.tryColdPublish(...)`** — re-emits the **persisted** `traceparent`/`tracestate` verbatim as Kafka headers (the cold poller may run after a restart with no live context), via `ProducerRecord`. **No-op when no recording span is active** (`OpenTelemetry.noop()` / sampled-out): `trace_parent` is `NULL`, no header is emitted, downstream roots its own trace — safe with or without a real OTel SDK, and forward-compatible with services not yet upgraded. ## Risk indicators | indicator | value | |---|---| | Shared starter (consumers) | 7 Java services consume `outbox-publisher` | | Schema change | +2 nullable columns (`trace_parent`, `trace_state`); local ddl-auto auto-applies; dev/stage/prod owners add a Flyway `ALTER TABLE` (documented in README) | | Kafka send shape | `send(topic,key,payload)` → `send(ProducerRecord)` in both relays (tests updated for the matcher ripple) | | New prod deps | none (`W3CTraceContextPropagator` is in the already-present `opentelemetry-api`; `ProducerRecord`/`Header` via the present `spring-kafka`/`kafka-clients`) | ## Verification - `mvn -pl outbox-publisher test` — green, **zero `[WARNING]`** (parent pom `failOnWarning=true`). - Unit tests assert: capture-at-write persists `trace_parent` on the row; the hot relay's `ProducerRecord` carries a `traceparent` header under an active SDK span; the cold poller re-emits a persisted `traceparent` verbatim (and emits none when null); accessor round-trip + truncation. - Deployed end-to-end (business op → client one `trace_id`) is gated on publishing the new `1.1.0-SNAPSHOT` to the Forgejo Maven registry + a family-service `--no-cache` rebuild (ci.yml does not yet auto-deploy — PR-PLATFORM-CI-1); tracked as the remaining verification step. ## Verdict **NEEDS_REVIEW** — shared-library change with a send-shape + schema ripple; backward/forward compatible; no new deps. README updated under "Distributed-trace carry through the outbox (GC-23 (b))". ## Footer im2be-platform-libs · GC-23(b) producer half · R0 (open) · outbox-publisher tests green/0-warn · 2026-06-01 
Preexisting flake (rule 18) surfaced while verifying GC-23 (b): Micrometer's
registry.gauge(name, stateObj, fn) holds stateObj by a WEAK reference, but
backlogAgeGaugeUsesOldestPending + backlogAgeZeroWhenNoPending constructed
the OutboxMetricsBinder WITHOUT retaining it. Under mvn verify's heavier
Testcontainers memory pressure the binder was GC'd before the gauge read →
the gauge reported NaN (expected 0.0 but was NaN). perTopicLatencyTimer...
already retained its binder and was unaffected.

Fix: keep a strong reference and Reference.reachabilityFence(binder) after
each gauge read so the binder stays live through it. Test-only; no product
change (the Spring context retains the real bean in production).

Verification: mvn -pl outbox-publisher verify -> 115 unit + 4 IT green, 0 warnings.
feat(outbox): carry W3C traceparent through the outbox to Kafka headers (GC-23 (b))
All checks were successful
im2be-platform-libs CI / mvn install (pull_request) Successful in 4m20s
Details
im2be-platform-libs CI / mvn verify (main only) (pull_request) Has been skipped
Details
6e2661d4b5 
Producer half of GC-23 (b) producer->relay origin. The outbox stores-and-
forwards, so a business event's trace previously ended at the outbox write and
the relay started a fresh trace. This carries the producing span's W3C
traceparent through to Kafka so the consuming relay (realtime-service #8,
already merged) continues the SAME trace_id: business event -> Kafka -> relay
-> Centrifugo -> client.

- OutboxTraceContext (new, package-private): W3C-explicit (W3CTraceContextProp-
  agator, not the configured propagator, so the traceparent header the polyglot
  consumer expects is always emitted per ADR-0013). captureCurrent / restore /
  currentKafkaHeaders / kafkaHeadersFor.
- OutboxRecord: + trace_parent (VARCHAR 64) + trace_state (VARCHAR 512) nullable
  columns + accessors (truncating setters). equals/hashCode unchanged (PK-only).
- OutboxPublisher.publish: captures the current W3C context at write (business
  span current) -> persists on the row; AFTER_COMMIT hot relay nests its
  publish span under the restored business context and emits that span's context
  as Kafka headers via ProducerRecord.
- OutboxPollerWorker.tryColdPublish: re-emits the PERSISTED traceparent/
  tracestate verbatim as Kafka headers (cold path runs without live context,
  possibly post-restart), via ProducerRecord.

No-op when no recording span is active (OpenTelemetry.noop()/sampled-out):
trace_parent NULL, no header, downstream roots its own trace. No new prod deps
(W3CTraceContextPropagator in opentelemetry-api; ProducerRecord/Header via
spring-kafka). Schema: local ddl-auto auto-applies the 2 columns; dev/stage/prod
owners add a Flyway ALTER TABLE (README documents it).

R0 findings (self): none blocking. Tests: send-shape mock ripple updated to
typed ProducerRecord matchers; hot+cold header-carry tests added; OutboxRecord
accessor tests added.

Verification:
- mvn -pl outbox-publisher verify -> 115 unit + 4 Testcontainers IT green, 0 [WARNING]
- OutboxPublisherIT exercises the real ProducerRecord send against a live Kafka broker.
pr-reviewer-bot commented 2026-06-01 16:09:04 +02:00
Member
Copy link

Superseded by round 2.

Show previous round

hib-pr-reviewer review — PR #24 (affinity-intelligence-rework/im2be-platform-libs)

Round 1 — head 6e2661d4b572, base main, trigger opened

TL;DR: CONDITIONAL_APPROVE — kept 2 minor findings (1 agreed-pair + 1 verified unique-to-A); no blocking issues.

Summary

Arbitration: PR #24 — W3C traceparent carry through outbox to Kafka headers

Memora lookup: No prior run history for this PR; no reusable submodule patterns stored. This is round 1.

Finding reconciliation:

  • F1 (unique to A) — OutboxPublisher.java scope-management gap. Verified via Read at lines 469–500: scope = span.makeCurrent() is called at line 469, then traceHeaders = OutboxTraceContext.currentKafkaHeaders() at line 474 sits outside the try block that starts at line 475. The finally { scope.close(); } is at lines 498–500. If line 474 were to throw, the finally is never reached and the scope leaks. Finding confirmed; kept.

  • F2 (agreed A + B) — OutboxPublisherTest.java test gap. Verified via Read at lines 780–786: tryHotPublish is called with null, null for traceParent/traceState. The restore(traceParent, traceState) → spanBuilder.setParent(businessContext) path (non-null parent, same trace_id invariant) is never exercised. Both reviewers independently flag the same test at the same line with materially equivalent bodies. Finding confirmed; kept.

Kept 2 findings (1 agreed-pair + 1 verified unique-to-A). All minor. Verdict: CONDITIONAL_APPROVE.

Blast Radius

The diff touches 10 files across the outbox-publisher module — entity, two relay code paths (hot + cold), a new helper class, tests, and README. Impact is contained within a single library module; no cross-module API surface or protocol contracts are changed. Consuming services require a manual DDL migration (documented in README) but the library change itself is backward-compatible (new columns nullable, carry degrades gracefully to no-op).

BLAST_SCORE: 4/10

Risk Indicators

Indicator Value
Sensitive functions OutboxTraceContext.captureCurrent, OutboxTraceContext.restore, OutboxTraceContext.currentKafkaHeaders, OutboxTraceContext.kafkaHeadersFor
Migration touched —
Test delta —
Dependency changes —

CI status (head 6e2661d4b572)

Overall: ✓ success

2 checks: 2 pending

Check State Link
im2be-platform-libs CI / mvn install (pull_request) ⏳ pending details
im2be-platform-libs CI / mvn verify (main only) (pull_request) ⏳ pending details

Findings (2)

[MINOR] Scope opened before try-block: currentKafkaHeaders() call at line 474 sits outside the finally that closes it

outbox-publisher/src/main/java/com/aim2be/platform/outbox/OutboxPublisher.java:474

At line 469, scope = span.makeCurrent() is called. The try block that contains finally { scope.close(); } (lines 475–500) does not start until line 475. OutboxTraceContext.currentKafkaHeaders() at line 474 sits in the unprotected gap between them. If that call were to throw (the OTel inject path does not in current practice, but it is outside the API contract), scope would never be closed — leaking the hot-publish span as the thread's current span indefinitely and preventing the span from ending.

Fix: move traceHeaders initialization to be the first statement inside the try block:

final Scope scope = span.makeCurrent();
final Context publishContext = Context.current();
try {
    final List<Header> traceHeaders = OutboxTraceContext.currentKafkaHeaders();
    kafkaTemplate.send(new ProducerRecord<>(
                    topic, null, aggregateKey(aggregateId), payload, traceHeaders))
            .whenComplete(...);
} catch (RuntimeException syncError) {
    ...
} finally {
    scope.close();
}

Context.current() at line 470 is infallible (it reads a thread-local) so it is safe to leave outside the try. currentKafkaHeaders() performs OTel map injection and is the first non-trivial call that could theoretically propagate an unchecked exception.

[MINOR] Hot-relay GC-23 test exercises only traceParent=null; the restore()→setParent() path with a non-null parent is never verified

outbox-publisher/src/test/java/com/aim2be/platform/outbox/OutboxPublisherTest.java:761

hotPublishCarriesTraceparentHeaderWhenSpanActive (lines 761–804) calls tryHotPublish with traceParent=null, traceState=null (lines 785–786). This exercises only the restore(null, null) → Context.root() branch — the hot-publish span starts a fresh trace and currentKafkaHeaders() captures that fresh trace's ID. The primary GC-23 (b) invariant — a non-null persisted traceParent causes restore() to rebuild the business context, the hot-publish span becomes its child (same trace_id, new span_id), and the Kafka header carries that same trace_id — is never asserted. This is the only path that would catch a regression in OutboxTraceContext.restore() (e.g. if the W3C extract call returned Context.root() due to an API change).

The cold-poller equivalent (coldPublishReEmitsPersistedTraceparentAndOmitsWhenAbsent) correctly covers its non-null path. Add a symmetric hot-relay test:

@Test
void hotPublishKafkaHeaderSharesTraceIdWithSuppliedParentTraceparent() {
    final InMemorySpanExporter exporter = InMemorySpanExporter.create();
    final SdkTracerProvider tp = SdkTracerProvider.builder()
            .addSpanProcessor(SimpleSpanProcessor.create(exporter)).build();
    try {
        final OpenTelemetrySdk sdk = OpenTelemetrySdk.builder().setTracerProvider(tp).build();
        // Capture a business span's traceparent as it would be persisted at write time.
        final Span biz = sdk.getTracer("test").spanBuilder("business").startSpan();
        final String persistedTp;
        try (Scope sc = biz.makeCurrent()) {
            persistedTp = OutboxTraceContext.captureCurrent().get(OutboxTraceContext.TRACEPARENT);
        } finally { biz.end(); }
        final String businessTraceId = persistedTp.split("-")[1];

        final OutboxPublisher publisher = newPublisherWithOtel(sdk);
        when(kafkaTemplate.send(ArgumentMatchers.<ProducerRecord<byte[], byte[]>>any()))
                .thenReturn(CompletableFuture.completedFuture(
                        (SendResult<byte[], byte[]>) mock(SendResult.class)));

        publisher.tryHotPublish("t", new byte[]{1}, UUID.randomUUID(),
                UUID.randomUUID(), persistedTp, null);

        final ArgumentCaptor<ProducerRecord<byte[], byte[]>> cap =
                ArgumentCaptor.forClass(ProducerRecord.class);
        verify(kafkaTemplate).send(cap.capture());
        final Header h = cap.getValue().headers().lastHeader("traceparent");
        assertThat(h).as("traceparent header must be present").isNotNull();
        assertThat(new String(h.value(), StandardCharsets.UTF_8).split("-")[1])
                .as("hot relay must continue the SAME trace_id as the business span")
                .isEqualTo(businessTraceId);
    } finally { tp.close(); }
}

Verdict

CONDITIONAL_APPROVE

hib-pr-reviewer • round 1 • 2 findings (2m) • 2026-06-01T14:07:20.615Z → 2026-06-01T14:09:04.195Z • posted-as: pr-reviewer-bot • model: auto

<!-- hib-pr-reviewer collapsed --> > _Superseded by round 2._ <details> <summary>Show previous round</summary> <!-- hib-pr-reviewer round:1 --> ## hib-pr-reviewer review — PR #24 (affinity-intelligence-rework/im2be-platform-libs) **Round 1** — head `6e2661d4b572`, base `main`, trigger `opened` **TL;DR:** CONDITIONAL_APPROVE — kept 2 minor findings (1 agreed-pair + 1 verified unique-to-A); no blocking issues. ### Summary ## Arbitration: PR #24 — W3C traceparent carry through outbox to Kafka headers **Memora lookup**: No prior run history for this PR; no reusable submodule patterns stored. This is round 1. **Finding reconciliation**: - **F1 (unique to A)** — `OutboxPublisher.java` scope-management gap. Verified via `Read` at lines 469–500: `scope = span.makeCurrent()` is called at line 469, then `traceHeaders = OutboxTraceContext.currentKafkaHeaders()` at line 474 sits *outside* the `try` block that starts at line 475. The `finally { scope.close(); }` is at lines 498–500. If line 474 were to throw, the `finally` is never reached and the scope leaks. **Finding confirmed; kept.** - **F2 (agreed A + B)** — `OutboxPublisherTest.java` test gap. Verified via `Read` at lines 780–786: `tryHotPublish` is called with `null, null` for `traceParent`/`traceState`. The `restore(traceParent, traceState) → spanBuilder.setParent(businessContext)` path (non-null parent, same `trace_id` invariant) is never exercised. Both reviewers independently flag the same test at the same line with materially equivalent bodies. **Finding confirmed; kept.** Kept 2 findings (1 agreed-pair + 1 verified unique-to-A). All minor. Verdict: CONDITIONAL_APPROVE. ### Blast Radius The diff touches 10 files across the outbox-publisher module — entity, two relay code paths (hot + cold), a new helper class, tests, and README. Impact is contained within a single library module; no cross-module API surface or protocol contracts are changed. Consuming services require a manual DDL migration (documented in README) but the library change itself is backward-compatible (new columns nullable, carry degrades gracefully to no-op). **BLAST_SCORE: 4/10** ### Risk Indicators | Indicator | Value | |---|---| | Sensitive functions | `OutboxTraceContext.captureCurrent`, `OutboxTraceContext.restore`, `OutboxTraceContext.currentKafkaHeaders`, `OutboxTraceContext.kafkaHeadersFor` | | Migration touched | — | | Test delta | — | | Dependency changes | — | ### CI status (head `6e2661d4b572`) **Overall: ✓ success** 2 checks: 2 pending | Check | State | Link | |---|---|---| | im2be-platform-libs CI / mvn install (pull_request) | ⏳ pending | [details](/affinity-intelligence-rework/im2be-platform-libs/actions/runs/120/jobs/0) | | im2be-platform-libs CI / mvn verify (main only) (pull_request) | ⏳ pending | [details](/affinity-intelligence-rework/im2be-platform-libs/actions/runs/120/jobs/1) | ### Findings (2) #### **[MINOR]** Scope opened before try-block: `currentKafkaHeaders()` call at line 474 sits outside the finally that closes it _outbox-publisher/src/main/java/com/aim2be/platform/outbox/OutboxPublisher.java:474_ At line 469, `scope = span.makeCurrent()` is called. The `try` block that contains `finally { scope.close(); }` (lines 475–500) does not start until line 475. `OutboxTraceContext.currentKafkaHeaders()` at line 474 sits in the unprotected gap between them. If that call were to throw (the OTel inject path does not in current practice, but it is outside the API contract), `scope` would never be closed — leaking the hot-publish span as the thread's current span indefinitely and preventing the span from ending. Fix: move `traceHeaders` initialization to be the first statement *inside* the `try` block: ```java final Scope scope = span.makeCurrent(); final Context publishContext = Context.current(); try { final List<Header> traceHeaders = OutboxTraceContext.currentKafkaHeaders(); kafkaTemplate.send(new ProducerRecord<>( topic, null, aggregateKey(aggregateId), payload, traceHeaders)) .whenComplete(...); } catch (RuntimeException syncError) { ... } finally { scope.close(); } ``` `Context.current()` at line 470 is infallible (it reads a thread-local) so it is safe to leave outside the try. `currentKafkaHeaders()` performs OTel map injection and is the first non-trivial call that could theoretically propagate an unchecked exception. #### **[MINOR]** Hot-relay GC-23 test exercises only `traceParent=null`; the `restore()→setParent()` path with a non-null parent is never verified _outbox-publisher/src/test/java/com/aim2be/platform/outbox/OutboxPublisherTest.java:761_ `hotPublishCarriesTraceparentHeaderWhenSpanActive` (lines 761–804) calls `tryHotPublish` with `traceParent=null, traceState=null` (lines 785–786). This exercises only the `restore(null, null) → Context.root()` branch — the hot-publish span starts a fresh trace and `currentKafkaHeaders()` captures that fresh trace's ID. The primary GC-23 (b) invariant — a non-null persisted `traceParent` causes `restore()` to rebuild the business context, the hot-publish span becomes its child (same `trace_id`, new `span_id`), and the Kafka header carries that same `trace_id` — is never asserted. This is the only path that would catch a regression in `OutboxTraceContext.restore()` (e.g. if the W3C `extract` call returned `Context.root()` due to an API change). The cold-poller equivalent (`coldPublishReEmitsPersistedTraceparentAndOmitsWhenAbsent`) correctly covers its non-null path. Add a symmetric hot-relay test: ```java @Test void hotPublishKafkaHeaderSharesTraceIdWithSuppliedParentTraceparent() { final InMemorySpanExporter exporter = InMemorySpanExporter.create(); final SdkTracerProvider tp = SdkTracerProvider.builder() .addSpanProcessor(SimpleSpanProcessor.create(exporter)).build(); try { final OpenTelemetrySdk sdk = OpenTelemetrySdk.builder().setTracerProvider(tp).build(); // Capture a business span's traceparent as it would be persisted at write time. final Span biz = sdk.getTracer("test").spanBuilder("business").startSpan(); final String persistedTp; try (Scope sc = biz.makeCurrent()) { persistedTp = OutboxTraceContext.captureCurrent().get(OutboxTraceContext.TRACEPARENT); } finally { biz.end(); } final String businessTraceId = persistedTp.split("-")[1]; final OutboxPublisher publisher = newPublisherWithOtel(sdk); when(kafkaTemplate.send(ArgumentMatchers.<ProducerRecord<byte[], byte[]>>any())) .thenReturn(CompletableFuture.completedFuture( (SendResult<byte[], byte[]>) mock(SendResult.class))); publisher.tryHotPublish("t", new byte[]{1}, UUID.randomUUID(), UUID.randomUUID(), persistedTp, null); final ArgumentCaptor<ProducerRecord<byte[], byte[]>> cap = ArgumentCaptor.forClass(ProducerRecord.class); verify(kafkaTemplate).send(cap.capture()); final Header h = cap.getValue().headers().lastHeader("traceparent"); assertThat(h).as("traceparent header must be present").isNotNull(); assertThat(new String(h.value(), StandardCharsets.UTF_8).split("-")[1]) .as("hot relay must continue the SAME trace_id as the business span") .isEqualTo(businessTraceId); } finally { tp.close(); } } ``` ### Verdict **CONDITIONAL_APPROVE** --- <sub>hib-pr-reviewer • round 1 • 2 findings (2m) • 2026-06-01T14:07:20.615Z → 2026-06-01T14:09:04.195Z • posted-as: pr-reviewer-bot • model: auto</sub> </details>
fix(outbox): R1 findings — close hot-relay scope-leak gap (F1) + test non-null-parent carry (F2)
All checks were successful
im2be-platform-libs CI / mvn install (pull_request) Successful in 3m30s
Details
im2be-platform-libs CI / mvn verify (main only) (pull_request) Has been skipped
Details
583977601c 
R1 verdict CONDITIONAL_APPROVE (2 minor, no blocking). Both addressed:

(1) MINOR OutboxPublisher.java — F1 scope-leak gap. currentKafkaHeaders() (+
the publishContext capture) sat in the unprotected gap between
span.makeCurrent() and the try whose finally closes the scope; an OTel-inject
throw (outside the API contract) would leak the current scope. Fix: moved both
captures INSIDE the try (still read while the scope is current, so they carry
the hot-publish span) — the finally now covers them.

(2) MINOR OutboxPublisherTest.java — F2 test gap. The restore(traceParent,
traceState)->setParent path (non-null parent, same-trace_id invariant) was
unexercised (all tryHotPublish calls passed null,null). Added
hotPublishNestsUnderRestoredParentWhenTraceParentProvided: passes a crafted
traceparent, asserts the emitted Kafka header keeps the parent's trace_id with
a new span_id (the hot-publish span nests under the business trace).

Ripple (rule 63): F1 also closes the pre-existing publishContext-in-the-gap
risk, not just the flagged line.

Verification: mvn -pl outbox-publisher verify -> 116 unit + 4 Testcontainers IT
green, 0 [WARNING].
hibryda commented 2026-06-01 16:13:29 +02:00
Author
Owner
Copy link

R1 findings addressed — head 5839776

TL;DR: Both R1 MINOR findings fixed; mvn -pl outbox-publisher verify green (116 unit + 4 Testcontainers IT, 0 [WARNING]). Re-review (R2) requested.

Summary: R1 = CONDITIONAL_APPROVE (2 minor, no blocking). Addressed at 5839776; no carried conditions.

Findings resolved

id sev where resolution
F1 MINOR OutboxPublisher.java hot relay Moved currentKafkaHeaders() + the publishContext capture INSIDE the try, so finally { scope.close(); } covers them — closes the makeCurrent()→try gap where an OTel-inject throw would have leaked the current scope. Rule-63 ripple: also protects the pre-existing publishContext capture, not just the flagged line.
F2 MINOR OutboxPublisherTest.java Added hotPublishNestsUnderRestoredParentWhenTraceParentProvided — passes a crafted traceparent, asserts the emitted Kafka header keeps the parent's trace_id with a NEW span_id (exercises the restore(...)→setParent non-null-parent path).

Verification: mvn -pl outbox-publisher verify → 116 unit + 4 Testcontainers IT green, 0 [WARNING]; OutboxPublisherIT exercises the real ProducerRecord send vs a live Kafka broker.

Verdict: NEEDS_REVIEW (R2).

Footer: im2be-platform-libs · GC-23(b) producer half · R1→fix · 116u+4it green · 2026-06-01

## R1 findings addressed — head `5839776` **TL;DR:** Both R1 MINOR findings fixed; `mvn -pl outbox-publisher verify` green (116 unit + 4 Testcontainers IT, 0 `[WARNING]`). Re-review (R2) requested. **Summary:** R1 = CONDITIONAL_APPROVE (2 minor, no blocking). Addressed at `5839776`; no carried conditions. **Findings resolved** | id | sev | where | resolution | |----|-----|-------|------------| | F1 | MINOR | `OutboxPublisher.java` hot relay | Moved `currentKafkaHeaders()` + the `publishContext` capture INSIDE the `try`, so `finally { scope.close(); }` covers them — closes the `makeCurrent()`→`try` gap where an OTel-inject throw would have leaked the current scope. Rule-63 ripple: also protects the pre-existing `publishContext` capture, not just the flagged line. | | F2 | MINOR | `OutboxPublisherTest.java` | Added `hotPublishNestsUnderRestoredParentWhenTraceParentProvided` — passes a crafted `traceparent`, asserts the emitted Kafka header keeps the parent's `trace_id` with a NEW span_id (exercises the `restore(...)→setParent` non-null-parent path). | **Verification:** `mvn -pl outbox-publisher verify` → 116 unit + 4 Testcontainers IT green, 0 `[WARNING]`; `OutboxPublisherIT` exercises the real `ProducerRecord` send vs a live Kafka broker. **Verdict:** NEEDS_REVIEW (R2). **Footer:** im2be-platform-libs · GC-23(b) producer half · R1→fix · 116u+4it green · 2026-06-01
pr-reviewer-bot commented 2026-06-01 16:18:29 +02:00
Member
Copy link

Superseded by round 3.

Show previous round

hib-pr-reviewer review — PR #24 (affinity-intelligence-rework/im2be-platform-libs)

Round 2 — head 583977601c50, base main, trigger synchronize

TL;DR: CONDITIONAL_APPROVE — 1 agreed minor finding kept (no OutboxTraceContextTest for boundary cases); both R1 findings confirmed resolved.

Summary

Round 2 arbitration — 1 agreed finding kept

Memora persistence

Attempted to persist run summary to Memora; all memory_create calls failed with invalid_input: Tag 'memora/issues' is not in the allowed tag list regardless of tag parameters provided. Proceeding with reconciliation output only.

R1 findings

Both A and B confirm R1 findings resolved:

  • F1 (scope-before-try) — fixed; publishContext/traceHeaders moved inside the try block (lines 467–476 of OutboxPublisher.java, confirmed via diff).
  • F2 (non-null parent test) — fixed; hotPublishNestsUnderRestoredParentWhenTraceParentProvided added (line 815+ of OutboxPublisherTest.java).

Finding reconciliation

Agreed (both A and B): Both cite OutboxTraceContext.java line 112 with materially-equivalent bodies — no dedicated direct unit test for OutboxTraceContext boundary cases. Verified: Glob finds no OutboxTraceContextTest.java; Grep finds OutboxTraceContext appears in test sources only once (as a comment in OutboxPublisherTest.java). Read of OutboxTraceContext.java confirms restore() at line 112 delegates malformed-but-non-blank input directly to W3C.extract() with no test asserting the silent-fallback contract. B's broader body (3 cases: malformed restore, captureCurrent noop, kafkaHeadersFor null) is adopted as it is a strict superset of A's 1-case claim and all three cases are independently verified against the code at lines 96–165. Kept as 1 merged minor finding.

Blast Radius

The diff adds OutboxTraceContext (a new shared utility) and wires it into both the hot-relay and cold-poller publish paths in OutboxPublisher and OutboxPollerWorker. Every service that depends on im2be-platform-libs and uses the outbox will pick up this carry automatically. The surface is bounded to the outbox module but the library is a platform shared dependency, so a regression would affect all consuming services.

BLAST_SCORE: 5/10

Risk Indicators

Indicator Value
Sensitive functions OutboxTraceContext.restore, OutboxTraceContext.captureCurrent, OutboxTraceContext.currentKafkaHeaders, OutboxTraceContext.kafkaHeadersFor, OutboxPublisher.tryHotPublish, OutboxPollerWorker.tryColdPublish
Migration touched —
Test delta —
Dependency changes —

CI status (head 583977601c50)

Overall: ✓ success

2 checks: 2 pending

Check State Link
im2be-platform-libs CI / mvn install (pull_request) ⏳ pending details
im2be-platform-libs CI / mvn verify (main only) (pull_request) ⏳ pending details

Findings (1)

[MINOR] No dedicated unit test for OutboxTraceContext — three boundary cases are unasserted

outbox-publisher/src/main/java/com/aim2be/platform/outbox/OutboxTraceContext.java:112

OutboxTraceContext is a new, non-trivial utility class (166 lines, zero prior tests). The only test coverage it receives is transitive — through OutboxPublisherTest and OutboxPollerWorkerTest. Three boundary cases are exercised by neither caller test:

  1. restore(malformed, null) — a syntactically invalid traceparent (e.g. "garbage", "00-short", or a value truncated by OutboxRecord.TRACE_PARENT_MAX_LENGTH=64 that exceeds the W3C fixed-55-char format) reaches line 121 (W3C.extract(...)) whose OTel spec contract is to silently return Context.root() — correct fallback — but that contract is nowhere asserted. A future OTel upgrade that changed extractor behaviour on malformed input would pass all tests while breaking trace isolation.
  2. captureCurrent() with the noop SDK — should return an empty map (no traceparent entry), so callers correctly persist NULL rather than a garbage header. Currently only implied by indirect null-parent Kafka-header tests, not verified directly.
  3. kafkaHeadersFor(null, null) — should return an empty list; kafkaHeadersFor(traceParent, null) should return exactly one header (no tracestate). The null-guard logic at lines 156–163 is correct but unasserted.

Recommended fix: add OutboxTraceContextTest.java:

@ParameterizedTest
@ValueSource(strings = {"garbage", "00-short", "00-" + "x".repeat(32) + "-" + "y".repeat(16) + "-ZZ"})
void restoreWithMalformedTraceparentFallsBackToRoot(String bad) {
    Context ctx = OutboxTraceContext.restore(bad, null);
    assertThat(Span.fromContext(ctx).getSpanContext().isValid()).isFalse();
}

@Test void captureCurrentWithNoopSdkReturnsEmptyMap() {
    assertThat(OutboxTraceContext.captureCurrent()).isEmpty();
}

@Test void kafkaHeadersForNullParentReturnsEmptyList() {
    assertThat(OutboxTraceContext.kafkaHeadersFor(null, "vendor=foo")).isEmpty();
}

@Test void kafkaHeadersForValidParentNullStateReturnsSingleHeader() {
    List<Header> headers = OutboxTraceContext.kafkaHeadersFor("00-" + "a".repeat(32) + "-" + "b".repeat(16) + "-01", null);
    assertThat(headers).hasSize(1);
    assertThat(headers.get(0).key()).isEqualTo("traceparent");
}

This pins the "no regression on bad DB data" and "no-op SDK produces no header" guarantees that the code already implements but does not assert.

Verdict

CONDITIONAL_APPROVE

hib-pr-reviewer • round 2 • 1 finding (1m) • 2026-06-01T14:16:39.679Z → 2026-06-01T14:18:28.811Z • posted-as: pr-reviewer-bot • model: auto

<!-- hib-pr-reviewer collapsed --> > _Superseded by round 3._ <details> <summary>Show previous round</summary> <!-- hib-pr-reviewer round:2 --> ## hib-pr-reviewer review — PR #24 (affinity-intelligence-rework/im2be-platform-libs) **Round 2** — head `583977601c50`, base `main`, trigger `synchronize` **TL;DR:** CONDITIONAL_APPROVE — 1 agreed minor finding kept (no OutboxTraceContextTest for boundary cases); both R1 findings confirmed resolved. ### Summary ## Round 2 arbitration — 1 agreed finding kept ### Memora persistence Attempted to persist run summary to Memora; all `memory_create` calls failed with `invalid_input: Tag 'memora/issues' is not in the allowed tag list` regardless of tag parameters provided. Proceeding with reconciliation output only. ### R1 findings Both A and B confirm R1 findings resolved: - **F1 (scope-before-try)** — fixed; `publishContext`/`traceHeaders` moved inside the `try` block (lines 467–476 of `OutboxPublisher.java`, confirmed via diff). - **F2 (non-null parent test)** — fixed; `hotPublishNestsUnderRestoredParentWhenTraceParentProvided` added (line 815+ of `OutboxPublisherTest.java`). ### Finding reconciliation **Agreed (both A and B):** Both cite `OutboxTraceContext.java` line 112 with materially-equivalent bodies — no dedicated direct unit test for `OutboxTraceContext` boundary cases. Verified: `Glob` finds no `OutboxTraceContextTest.java`; `Grep` finds `OutboxTraceContext` appears in test sources only once (as a comment in `OutboxPublisherTest.java`). `Read` of `OutboxTraceContext.java` confirms `restore()` at line 112 delegates malformed-but-non-blank input directly to `W3C.extract()` with no test asserting the silent-fallback contract. B's broader body (3 cases: malformed restore, captureCurrent noop, kafkaHeadersFor null) is adopted as it is a strict superset of A's 1-case claim and all three cases are independently verified against the code at lines 96–165. **Kept as 1 merged minor finding.** ### Blast Radius The diff adds `OutboxTraceContext` (a new shared utility) and wires it into both the hot-relay and cold-poller publish paths in `OutboxPublisher` and `OutboxPollerWorker`. Every service that depends on `im2be-platform-libs` and uses the outbox will pick up this carry automatically. The surface is bounded to the outbox module but the library is a platform shared dependency, so a regression would affect all consuming services. **BLAST_SCORE: 5/10** ### Risk Indicators | Indicator | Value | |---|---| | Sensitive functions | `OutboxTraceContext.restore`, `OutboxTraceContext.captureCurrent`, `OutboxTraceContext.currentKafkaHeaders`, `OutboxTraceContext.kafkaHeadersFor`, `OutboxPublisher.tryHotPublish`, `OutboxPollerWorker.tryColdPublish` | | Migration touched | — | | Test delta | — | | Dependency changes | — | ### CI status (head `583977601c50`) **Overall: ✓ success** 2 checks: 2 pending | Check | State | Link | |---|---|---| | im2be-platform-libs CI / mvn install (pull_request) | ⏳ pending | [details](/affinity-intelligence-rework/im2be-platform-libs/actions/runs/121/jobs/0) | | im2be-platform-libs CI / mvn verify (main only) (pull_request) | ⏳ pending | [details](/affinity-intelligence-rework/im2be-platform-libs/actions/runs/121/jobs/1) | ### Findings (1) #### **[MINOR]** No dedicated unit test for `OutboxTraceContext` — three boundary cases are unasserted _outbox-publisher/src/main/java/com/aim2be/platform/outbox/OutboxTraceContext.java:112_ `OutboxTraceContext` is a new, non-trivial utility class (166 lines, zero prior tests). The only test coverage it receives is transitive — through `OutboxPublisherTest` and `OutboxPollerWorkerTest`. Three boundary cases are exercised by neither caller test: 1. **`restore(malformed, null)`** — a syntactically invalid `traceparent` (e.g. `"garbage"`, `"00-short"`, or a value truncated by `OutboxRecord.TRACE_PARENT_MAX_LENGTH=64` that exceeds the W3C fixed-55-char format) reaches line 121 (`W3C.extract(...)`) whose OTel spec contract is to silently return `Context.root()` — correct fallback — but that contract is nowhere asserted. A future OTel upgrade that changed extractor behaviour on malformed input would pass all tests while breaking trace isolation. 2. **`captureCurrent()` with the noop SDK** — should return an empty map (no `traceparent` entry), so callers correctly persist `NULL` rather than a garbage header. Currently only implied by indirect null-parent Kafka-header tests, not verified directly. 3. **`kafkaHeadersFor(null, null)`** — should return an empty list; `kafkaHeadersFor(traceParent, null)` should return exactly one header (no `tracestate`). The null-guard logic at lines 156–163 is correct but unasserted. **Recommended fix:** add `OutboxTraceContextTest.java`: ```java @ParameterizedTest @ValueSource(strings = {"garbage", "00-short", "00-" + "x".repeat(32) + "-" + "y".repeat(16) + "-ZZ"}) void restoreWithMalformedTraceparentFallsBackToRoot(String bad) { Context ctx = OutboxTraceContext.restore(bad, null); assertThat(Span.fromContext(ctx).getSpanContext().isValid()).isFalse(); } @Test void captureCurrentWithNoopSdkReturnsEmptyMap() { assertThat(OutboxTraceContext.captureCurrent()).isEmpty(); } @Test void kafkaHeadersForNullParentReturnsEmptyList() { assertThat(OutboxTraceContext.kafkaHeadersFor(null, "vendor=foo")).isEmpty(); } @Test void kafkaHeadersForValidParentNullStateReturnsSingleHeader() { List<Header> headers = OutboxTraceContext.kafkaHeadersFor("00-" + "a".repeat(32) + "-" + "b".repeat(16) + "-01", null); assertThat(headers).hasSize(1); assertThat(headers.get(0).key()).isEqualTo("traceparent"); } ``` This pins the "no regression on bad DB data" and "no-op SDK produces no header" guarantees that the code already implements but does not assert. ### Verdict **CONDITIONAL_APPROVE** --- <sub>hib-pr-reviewer • round 2 • 1 finding (1m) • 2026-06-01T14:16:39.679Z → 2026-06-01T14:18:28.811Z • posted-as: pr-reviewer-bot • model: auto</sub> </details>
test(outbox): add OutboxTraceContextTest boundary coverage (R2 finding)
All checks were successful
im2be-platform-libs CI / mvn install (pull_request) Successful in 3m28s
Details
im2be-platform-libs CI / mvn verify (main only) (pull_request) Has been skipped
Details
16aa8c17de 
R2 verdict CONDITIONAL_APPROVE — both R1 findings confirmed resolved; 1 new
agreed minor: OutboxTraceContext had no dedicated test for its boundary cases
(its no-op/null-path safety was only exercised indirectly via the publisher +
poller). Added 11 focused tests pinning the degenerate inputs the carry relies
on to be safe ahead of / alongside un-instrumented services:

- captureCurrent() / currentKafkaHeaders() with no active span -> empty.
- restore(null|blank, ...) -> root context (invalid span); restore(validTP, _)
  -> valid REMOTE span context with the right trace_id/span_id; restore carries
  a valid W3C tracestate.
- kafkaHeadersFor(null|blank, _) -> empty; (tp, null) -> 1 header; (tp, ts) ->
  2 headers; (tp, blank-ts) -> tracestate skipped.

The active-recording-span emit paths stay covered against the SDK in
OutboxPublisherTest.

Verification: mvn -pl outbox-publisher test -> 127 unit (was 116, +11), 0 [WARNING].
hibryda commented 2026-06-01 16:20:44 +02:00
Author
Owner
Copy link

R2 finding addressed — head 16aa8c1

TL;DR: R2's lone minor (no OutboxTraceContextTest) resolved; mvn -pl outbox-publisher test → 127 unit (was 116, +11), 0 [WARNING]. Both R1 findings already confirmed resolved by R2. Re-review (R3) requested.

Finding resolved

id sev where resolution
R2-1 MINOR OutboxTraceContext boundary coverage Added OutboxTraceContextTest (11 tests) pinning the no-op/null/blank paths the carry relies on: captureCurrent()/currentKafkaHeaders() empty with no active span; restore(null|blank) → root context, restore(validTP) → valid REMOTE span context (right trace_id/span_id) + tracestate round-trip; kafkaHeadersFor null/blank → empty, (tp) → 1 header, (tp,ts) → 2, (tp, blank-ts) → tracestate skipped. Active-span emit paths remain covered against the SDK in OutboxPublisherTest.

Verification: mvn -pl outbox-publisher test → 127 unit, 0 [WARNING] (IT unchanged: 4 Testcontainers IT green in the prior verify).

Verdict: NEEDS_REVIEW (R3) — no carried conditions; R1 (F1+F2) + R2 (boundary tests) all resolved.

Footer: im2be-platform-libs · GC-23(b) producer half · R2→fix · 127u green · 2026-06-01

## R2 finding addressed — head `16aa8c1` **TL;DR:** R2's lone minor (no `OutboxTraceContextTest`) resolved; `mvn -pl outbox-publisher test` → 127 unit (was 116, +11), 0 `[WARNING]`. Both R1 findings already confirmed resolved by R2. Re-review (R3) requested. **Finding resolved** | id | sev | where | resolution | |----|-----|-------|------------| | R2-1 | MINOR | `OutboxTraceContext` boundary coverage | Added `OutboxTraceContextTest` (11 tests) pinning the no-op/null/blank paths the carry relies on: `captureCurrent()`/`currentKafkaHeaders()` empty with no active span; `restore(null\|blank)` → root context, `restore(validTP)` → valid REMOTE span context (right trace_id/span_id) + tracestate round-trip; `kafkaHeadersFor` null/blank → empty, (tp) → 1 header, (tp,ts) → 2, (tp, blank-ts) → tracestate skipped. Active-span emit paths remain covered against the SDK in `OutboxPublisherTest`. | **Verification:** `mvn -pl outbox-publisher test` → 127 unit, 0 `[WARNING]` (IT unchanged: 4 Testcontainers IT green in the prior `verify`). **Verdict:** NEEDS_REVIEW (R3) — no carried conditions; R1 (F1+F2) + R2 (boundary tests) all resolved. **Footer:** im2be-platform-libs · GC-23(b) producer half · R2→fix · 127u green · 2026-06-01
pr-reviewer-bot commented 2026-06-01 16:25:36 +02:00
Member
Copy link

hib-pr-reviewer review — PR #24 (affinity-intelligence-rework/im2be-platform-libs)

Round 3 — head 16aa8c17de14, base main, trigger synchronize

TL;DR: CONDITIONAL_APPROVE — kept 1 minor agreed/verified finding (unique-to-A, confirmed by file read); 0 findings from B; all 3 prior-round issues resolved.

Summary

Round 3 Arbitration

Kept 1 finding (minor, unique-to-A, verified).
Dropped 0 findings.

Prior-round findings: all resolved

  • R1/R2 F1 — scope-before-try leak (OutboxPublisher.java): Both reviewers confirm fixed — publishContext and currentKafkaHeaders() are now inside the try block.
  • R1 F2 — hot-relay non-null-parent path untested (OutboxPublisherTest.java:815): Both reviewers confirm hotPublishNestsUnderRestoredParentWhenTraceParentProvided is present and exercises the full invariant.
  • R2 F1 — no dedicated OutboxTraceContext unit tests: Both reviewers confirm OutboxTraceContextTest.java (11 tests) covers the full boundary matrix.

Unique-to-A finding: verified and kept

Read OutboxPollerWorkerTest.java lines 340–382: line 353 sets withTrace.setTraceParent(persistedTraceparent) only. No setTraceState(...) call. No assertion on a tracestate header. Production code at OutboxPollerWorker.java lines 218–219 calls kafkaHeadersFor(record.getTraceParent(), record.getTraceState()) with both arguments. The wiring from record.getTraceState() → Kafka header is untested at the integration level. The unit-level logic is covered in OutboxTraceContextTest.kafkaHeadersForTraceParentAndStateEmitsBothHeaders, so risk is low — kept at minor.

Memora note

Tag allowlist blocked persistence (tags pr-review-run and default auto-tags both rejected); Memora write skipped.

Blast Radius

The diff touches 11 files across the outbox-publisher main and test trees. The production surface is an exported library used by consuming services (OutboxRecord schema change, OutboxTraceContext new class, OutboxPublisher and OutboxPollerWorker wiring changes). The schema change (new nullable columns) requires a manual migration step for services with explicit Flyway/Liquibase migrations, as documented in README.md.

BLAST_SCORE: 4/10

Risk Indicators

Indicator Value
Sensitive functions OutboxTraceContext.captureCurrent, OutboxTraceContext.restore, OutboxTraceContext.kafkaHeadersFor, OutboxTraceContext.currentKafkaHeaders
Migration touched —
Test delta —
Dependency changes —

CI status (head 16aa8c17de14)

Overall: ✓ success

2 checks: 2 pending

Check State Link
im2be-platform-libs CI / mvn install (pull_request) ⏳ pending details
im2be-platform-libs CI / mvn verify (main only) (pull_request) ⏳ pending details

Findings (1)

[MINOR] Cold-poller test exercises only traceParent; the traceState co-carry path is never asserted at the integration level

outbox-publisher/src/test/java/com/aim2be/platform/outbox/OutboxPollerWorkerTest.java:353

Line 353 sets withTrace.setTraceParent(persistedTraceparent) but never calls withTrace.setTraceState(...). The cold-poller sends kafkaHeadersFor(record.getTraceParent(), record.getTraceState()) (OutboxPollerWorker.java:218–219); the traceState argument only produces a header when it is non-blank. The test asserts the traceparent header is present and correct but has no assertion on tracestate, so a future refactor that drops or shadows the second argument would pass all tests silently.

Fix: add withTrace.setTraceState("vendorA=t61rcWkgMzE") after line 353, then assert first.headers().lastHeader("tracestate") is non-null and equals "vendorA=t61rcWkgMzE". The unit-level logic is already covered by OutboxTraceContextTest.kafkaHeadersForTraceParentAndStateEmitsBothHeaders, so this is a narrow wiring-level gap only.

Verdict

CONDITIONAL_APPROVE

hib-pr-reviewer • round 3 • 1 finding (1m) • 2026-06-01T14:24:05.179Z → 2026-06-01T14:25:36.580Z • posted-as: pr-reviewer-bot • model: auto

<!-- hib-pr-reviewer round:3 --> ## hib-pr-reviewer review — PR #24 (affinity-intelligence-rework/im2be-platform-libs) **Round 3** — head `16aa8c17de14`, base `main`, trigger `synchronize` **TL;DR:** CONDITIONAL_APPROVE — kept 1 minor agreed/verified finding (unique-to-A, confirmed by file read); 0 findings from B; all 3 prior-round issues resolved. ### Summary ## Round 3 Arbitration **Kept 1 finding (minor, unique-to-A, verified).** **Dropped 0 findings.** ### Prior-round findings: all resolved - **R1/R2 F1 — scope-before-try leak** (OutboxPublisher.java): Both reviewers confirm fixed — `publishContext` and `currentKafkaHeaders()` are now inside the `try` block. - **R1 F2 — hot-relay non-null-parent path untested** (OutboxPublisherTest.java:815): Both reviewers confirm `hotPublishNestsUnderRestoredParentWhenTraceParentProvided` is present and exercises the full invariant. - **R2 F1 — no dedicated OutboxTraceContext unit tests**: Both reviewers confirm `OutboxTraceContextTest.java` (11 tests) covers the full boundary matrix. ### Unique-to-A finding: verified and kept Read `OutboxPollerWorkerTest.java` lines 340–382: line 353 sets `withTrace.setTraceParent(persistedTraceparent)` only. No `setTraceState(...)` call. No assertion on a `tracestate` header. Production code at `OutboxPollerWorker.java` lines 218–219 calls `kafkaHeadersFor(record.getTraceParent(), record.getTraceState())` with both arguments. The wiring from `record.getTraceState()` → Kafka header is untested at the integration level. The unit-level logic is covered in `OutboxTraceContextTest.kafkaHeadersForTraceParentAndStateEmitsBothHeaders`, so risk is low — kept at **minor**. ### Memora note Tag allowlist blocked persistence (tags `pr-review-run` and default auto-tags both rejected); Memora write skipped. ### Blast Radius The diff touches 11 files across the outbox-publisher main and test trees. The production surface is an exported library used by consuming services (OutboxRecord schema change, OutboxTraceContext new class, OutboxPublisher and OutboxPollerWorker wiring changes). The schema change (new nullable columns) requires a manual migration step for services with explicit Flyway/Liquibase migrations, as documented in README.md. **BLAST_SCORE: 4/10** ### Risk Indicators | Indicator | Value | |---|---| | Sensitive functions | `OutboxTraceContext.captureCurrent`, `OutboxTraceContext.restore`, `OutboxTraceContext.kafkaHeadersFor`, `OutboxTraceContext.currentKafkaHeaders` | | Migration touched | — | | Test delta | — | | Dependency changes | — | ### CI status (head `16aa8c17de14`) **Overall: ✓ success** 2 checks: 2 pending | Check | State | Link | |---|---|---| | im2be-platform-libs CI / mvn install (pull_request) | ⏳ pending | [details](/affinity-intelligence-rework/im2be-platform-libs/actions/runs/122/jobs/0) | | im2be-platform-libs CI / mvn verify (main only) (pull_request) | ⏳ pending | [details](/affinity-intelligence-rework/im2be-platform-libs/actions/runs/122/jobs/1) | ### Findings (1) #### **[MINOR]** Cold-poller test exercises only `traceParent`; the `traceState` co-carry path is never asserted at the integration level _outbox-publisher/src/test/java/com/aim2be/platform/outbox/OutboxPollerWorkerTest.java:353_ Line 353 sets `withTrace.setTraceParent(persistedTraceparent)` but never calls `withTrace.setTraceState(...)`. The cold-poller sends `kafkaHeadersFor(record.getTraceParent(), record.getTraceState())` (OutboxPollerWorker.java:218–219); the `traceState` argument only produces a header when it is non-blank. The test asserts the `traceparent` header is present and correct but has no assertion on `tracestate`, so a future refactor that drops or shadows the second argument would pass all tests silently. Fix: add `withTrace.setTraceState("vendorA=t61rcWkgMzE")` after line 353, then assert `first.headers().lastHeader("tracestate")` is non-null and equals `"vendorA=t61rcWkgMzE"`. The unit-level logic is already covered by `OutboxTraceContextTest.kafkaHeadersForTraceParentAndStateEmitsBothHeaders`, so this is a narrow wiring-level gap only. ### Verdict **CONDITIONAL_APPROVE** --- <sub>hib-pr-reviewer • round 3 • 1 finding (1m) • 2026-06-01T14:24:05.179Z → 2026-06-01T14:25:36.580Z • posted-as: pr-reviewer-bot • model: auto</sub>
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels
No items
No labels
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
2 participants
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference
affinity-intelligence-rework/im2be-platform-libs!24
No description provided.