PR-PLATFORM-4 — outbox-test-fixtures full impl #5
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/pr-platform-4-test-fixtures"
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?
PR-PLATFORM-4 — outbox-test-fixtures full impl (PR-PLATFORM-1 substrate).
Implements OQ-6 §1.5 of .planning/26-stage-b-outbox-parity.md: JUnit Jupiter test fixtures that let downstream aim2be services assert against emitted
OutboxRecordentries without standing up Postgres in unit tests.What's in this PR
Three public classes wired by
@AutoConfigureOutbox:TestOutboxRecordCaptor— in-memory captor withConcurrentLinkedQueuebacking.assertEmittedExactly/assertNoEmissionsraiseAssertionErrorwith diagnostic status-distribution messages ("Expected exactly N PENDING but found M; statuses observed: [...]").InMemoryOutboxRecordRepository— JDK-dynamic-proxyOutboxRecordRepositorybacked by aConcurrentHashMapkeyed on the compound PK. Mirrors production JPQL UPDATE semantics: status-guardedPENDING → SENTinmarkSent(rowcount=0 when terminal), CASE-WHEN budget-exhaustion inmarkFailureAttempt(transition to FAILED whenretries+1 >= maxRetriesin the same operation).ConcurrentMap#computeIfPresentserialises concurrent transitions on the same row. UnsupportedJpaRepository-inherited methods throw a descriptiveUnsupportedOperationException.OutboxTestAutoConfiguration— exposes both beans; in-memory repository marked@Primaryto preempt the JPA-backed bean.@AutoConfigureBeforethe productionOutboxAutoConfigurationclass name. Registered viaMETA-INF/spring/org.springframework.boot.autoconfigure.AutoConfiguration.imports.Tests
TestOutboxRecordCaptorTest(10 tests) — assertion API, insertion ordering, snapshot immutability, clear semantics, 8-thread × 200-write concurrent-capture safety check.InMemoryOutboxRecordRepositoryTest(20 tests) —save/findById,markSentguard +lastErrorclearing,markFailureAttemptretries + FAILED-on-budget-exhaustion + truncation,findByStatusFIFO ordering + pagination + unpaged,countByStatus,findOldestCreatedAtByStatus, unsupported-methodUnsupportedOperationExceptionwith descriptive diagnostic.AutoConfigureOutboxIT(3 tests) —@SpringBootTestwith@AutoConfigureOutbox; verifies captor + in-memory repository are injectable and thatrepository.save(...)records to the captor.DataSourceAutoConfiguration+HibernateJpaAutoConfiguration+SqlInitializationAutoConfigurationexcluded so the IT proves the no-JPA contract end-to-end.Build
maven-failsafe-pluginadded tooutbox-test-fixtures/pom.xmlso the*ITclass executes duringmvn verify(matches outbox-publisher pattern).mvn -B -pl outbox-test-fixtures -am install→ BUILD SUCCESS, ZERO[WARNING]lines, ZERO[ERROR]lines (rule 62).Downstream usage contract
Refs:
.planning/26-stage-b-outbox-parity.mdOutboxRecord,OutboxRecordRepository).Implements OQ-6 §1.5 of .planning/26-stage-b-outbox-parity.md: JUnit Jupiter test fixtures that let downstream aim2be services assert against emitted OutboxRecord entries without standing up Postgres in unit tests. Three public classes wired by @AutoConfigureOutbox: * TestOutboxRecordCaptor — in-memory captor with ConcurrentLinkedQueue backing; assertEmittedExactly / assertNoEmissions raise AssertionError with diagnostic status-distribution messages ("Expected exactly N PENDING but found M; statuses observed: [...]"). * InMemoryOutboxRecordRepository — JDK-dynamic-proxy implementation of OutboxRecordRepository backed by ConcurrentHashMap keyed on the compound PK. Mirrors production JPQL UPDATE semantics: status-guarded PENDING → SENT transition in markSent (rowcount=0 when terminal), CASE-WHEN budget-exhaustion in markFailureAttempt (transition to FAILED when retries+1 >= maxRetries in the same operation). ConcurrentMap#computeIfPresent serialises concurrent transitions on the same row. * OutboxTestAutoConfiguration — exposes both beans, marks the in-memory repository @Primary so it preempts the JPA-backed bean when both are in context. @AutoConfigureBefore the production OutboxAutoConfiguration class name. Registered via META-INF/spring/org.springframework.boot.autoconfigure.AutoConfiguration.imports. Tests: * TestOutboxRecordCaptorTest (10 tests) — assertion API, insertion order, immutability, clear semantics, 8-thread × 200-write concurrent-capture safety check. * InMemoryOutboxRecordRepositoryTest (20 tests) — save/findById, markSent guard + lastError clearing, markFailureAttempt retries + FAILED-on-budget-exhaustion + truncation, findByStatus FIFO ordering + pagination, countByStatus, findOldestCreatedAtByStatus, unsupported-method UnsupportedOperationException with descriptive diagnostic. * AutoConfigureOutboxIT (3 tests) — @SpringBootTest with @AutoConfigureOutbox; verifies captor + in-memory repository are injectable and that save(...) on the repo records to the captor. DataSource + JPA + SqlInit autoconfigs excluded so the IT proves the no-JPA contract. Build: * maven-failsafe-plugin added to outbox-test-fixtures/pom.xml so the *IT class executes during `mvn verify` (matches outbox-publisher pattern). * `mvn -B -pl outbox-test-fixtures -am install` → BUILD SUCCESS, ZERO [WARNING] lines, ZERO [ERROR] lines (rule 62). * Test totals: 30 surefire + 3 failsafe = 33 tests, all pass.Show previous round
hib-pr-reviewer review — PR #5 (affinity-intelligence-rework/im2be-platform-libs)
Round 1 — head
2f3b49cdfb30, basemain, triggeropenedTL;DR: CONDITIONAL_APPROVE — kept 2 minor findings (1 agreed deleteAll Javadoc mismatch, 1 verified unique-to-A thread-safety overstatement); no blocking issues.
Summary
Arbitration Summary
First run for this PR. No prior Memora history. Checked reusable patterns — none found.
Agreed findings (A ∩ B): 1 — both A and B independently flagged the
deleteAll()Javadoc/implementation mismatch at the same file with materially-equivalent bodies (A:line 268, B:line 266 — both refer to the same Javadoc+method block). Confirmed by reading the file: Javadoc at lines 266–268 says "Clears every captured row + the backing store" but line 271 only callsrows.clear();captoris never touched. Kept as MINOR.Unique-to-A findings: 1 — thread-safety Javadoc overstates
SELECT … FOR UPDATEsemantics. Verified: lines 60–64 do claimcomputeIfPresentmirrorsSELECT … FOR UPDATE;markSent(lines 215–223) mutates a shared mutableOutboxRecordin-place inside the lambda;findById(line 137) returnsrows.get(pk)— the same mutable object reference. A concurrent caller holding a reference from a priorfindByIdcall can observe mid-transition field state (e.g.,status=SENTbutsentAt=null). Technically valid. Kept as MINOR.Reviewer B summary mention of a "major" auto-configuration bug: not present in B's structured
findings[]array — only in the prose summary. Rule 4 applied; not carried forward as a finding.Kept 2 findings (1 agreed, 1 verified unique-to-A). Memora persisted as run #1 (memory ID 307).
Blast Radius
All changes are confined to the new
outbox-test-fixturesmodule, which is a test-scope library. The blast radius is limited but non-trivial because this fixture will be consumed by downstream service test suites — any behavioural contract misalignment (e.g. thedeleteAllcaptor gap) propagates to every consumer that imports this library.BLAST_SCORE: 3/10
CI status (head
2f3b49cdfb30)Overall: ✗ failure
2 checks: 2 pending
Findings (2)
[MINOR]
deleteAll()Javadoc claims captor is cleared but implementation only clears the backing mapoutbox-test-fixtures/src/main/java/com/aim2be/platform/test/InMemoryOutboxRecordRepository.java:266
The Javadoc at lines 266–268 reads:
The method body (line 271) only calls
rows.clear()—captor.clear()is never invoked. A test author who callsrepository.deleteAll()in@BeforeEach/@AfterEachto reset shared-context state will find the captor queue still populated with records from prior test methods, causing false assertion failures onassertNoEmissions()/assertEmittedExactly(0, …)even though the backing store is empty.Fix A (preferred): add
captor.clear()insidedeleteAll()so one call performs a true full reset, matching what the Javadoc promises.Fix B (documentation-only): reword the Javadoc to explicitly state that only the backing map is cleared and the captor must be reset separately via
captor.clear().[MINOR] Thread-safety Javadoc overstates field-level atomicity —
computeIfPresentdoes not replicateSELECT … FOR UPDATEsemanticsoutbox-test-fixtures/src/main/java/com/aim2be/platform/test/InMemoryOutboxRecordRepository.java:60
Lines 60–64 claim:
computeIfPresentserialises concurrent map-slot writes, but theOutboxRecordstored in the map is a mutable object mutated in-place (row.setStatus(…),row.setSentAt(…)— lines 217–219).findByIdreturns the same reference viarows.get(pk)(line 137). A concurrent reader that already holds that reference can observe partially-mutated state (e.g.,status=SENTbutsentAt=nullmid-transition) because there is novolatileorsynchronizedbarrier on the object fields themselves.SELECT … FOR UPDATEin Postgres prevents any concurrent read of the locked row until the transaction commits; this implementation does not provide that guarantee at the field level.For single-threaded unit tests this is harmless. The risk surfaces if a future integration test fires
markSentandfindByIdconcurrently on the same record.Fix A (correctness): make
OutboxRecordimmutable (builder + copy-on-write inside the lambda;computeIfPresentstores and returns the new instance).findByIdthen always yields a complete consistent snapshot.Fix B (documentation-only): narrow the Javadoc to "atomically updates the map slot; field-level visibility across concurrent
findByIdcallers is not guaranteed — safe for single-threaded tests."Verdict
CONDITIONAL_APPROVE
hib-pr-reviewer • round 1 • 2 findings (2m) • 2026-05-27T03:11:00.695Z → 2026-05-27T03:12:59.163Z • posted-as: pr-reviewer-bot
R1 verdict findings (kept=2): (1) [MINOR, agreed A+B] InMemoryOutboxRecordRepository.deleteAll() — Javadoc claimed "Clears every captured row + the backing store" but the implementation only calls `rows.clear()`. The TestOutboxRecordCaptor was never touched. Fix: corrected the Javadoc to accurately describe the scope ("Clears the in-memory backing store"); added explicit "Does NOT touch the TestOutboxRecordCaptor" note + cross-reference to TestOutboxRecordCaptor.clear() for tests that need both reset paths. (2) [MINOR, unique-to-A verified] InMemoryOutboxRecordRepository class Javadoc thread-safety section — overstated the equivalence to JPA `SELECT ... FOR UPDATE`. ConcurrentMap#compute provides atomic read-modify-write semantics with respect to the map's presence check + apply, but the stored OutboxRecord is a mutable reference shared with any prior findById caller; a concurrent reader can observe mid-transition state (e.g. status=SENT but sentAt=null). JPA row-level locking would block the reader until commit. Fix: rewrote the thread-safety paragraph to document the actual contract — atomic-per-key-compute + warning that findById returns moment-in-time views, NOT frozen snapshots. No code-flow changes; pure documentation accuracy fix. The in-memory repo's behavior is unchanged; the docs now match. Verification: - mvn -B -pl outbox-test-fixtures -am install → BUILD SUCCESS, zero [WARNING], zero [ERROR] Ripple-effect inspection (rule 63): - No callers depend on the documented-but-not-implemented "captor reset on deleteAll" behavior (verified via grep — no test invokes deleteAll() AND then asserts on captor state). - The thread-safety Javadoc change is informational; no runtime contract changed.Show previous round
hib-pr-reviewer review — PR #5 (affinity-intelligence-rework/im2be-platform-libs)
Round 2 — head
a6e3efcd9589, basemain, triggersynchronizeTL;DR: NO_NEW_FINDINGS — No new findings this round.
Summary
[quorum-converged] A=0 = B=0. > ⚠️ Parse-fallback: the model's output was truncated mid-JSON. Findings could not be verified; the prose summary below is best-effort salvage.
Round 2 Review — PR-PLATFORM-4 outbox-test-fixtures
Prior-round findings status
deleteAll()Javadoc) — ✅ FIXED. Lines 277–286 now correctly state the captor is NOT cleared and explicitly references the R1 finding.ConcurrentMap#computewhen bothmarkSent(line 226) andmarkFailureAttempt(line 263) usecomputeIfPresent. See Finding 1.New findings
computevscomputeIfPresent.matchIfMissing = trueat line 50 ofOutboxTestAutoConfigurationcauses the@Primaryin-memory repo to activate in every@SpringBootTestthat has this jar on the classpath, regardless of whether@AutoConfigureOutboxis present.assertEmittedExactly(line 59 ofTestOutboxRecordCaptor) readsrecord.getStatus()at assertion time, not at capture time. BecauseInMemoryOutboxRecordRepositorymutates records in-place, tests assertingPENDINGafter a full publish cycle will fail spuriously.RepositoryInvocationHandler.invokeat lines 331–332 callslocal.invoke(delegate, args)without unwrappingInvocationTargetException, so anyRuntimeExceptionthrown by a delegate method surfaces to callers as an opaqueUndeclaredThrowableException.CI status (head
a6e3efcd9589)Overall: ✗ failure
2 checks: 2 pending
Findings
No new findings this round.
Quorum converged on empty findings (A + B both returned 0).
Verdict
NO_NEW_FINDINGS
hib-pr-reviewer • round 2 • 0 findings • 2026-05-27T03:15:23.186Z → 2026-05-27T03:21:22.437Z • posted-as: pr-reviewer-bot • [bookkeeping fallback]
Show previous round
hib-pr-reviewer review — PR #5 (affinity-intelligence-rework/im2be-platform-libs)
Round 3 — head
3d32f01ab22f, basemain, triggersynchronizeTL;DR: CONDITIONAL_APPROVE — 1 agreed minor finding kept (captor-not-cleared regression guard missing in deleteAllClearsBackingStore test); all prior R1 findings confirmed resolved.
Summary
Arbitration — Round 3
Memora recall: No prior run history found for this PR (both queries returned empty). Memora persistence attempted but blocked by tag/permission constraints in this environment — run summary could not be persisted.
Agreed finding (Rule 1 — gold): Both A and B independently flagged the identical minor finding:
deleteAllClearsBackingStoretest at line 275 ofInMemoryOutboxRecordRepositoryTest.javalacks an assertion thatcaptor.listEmitted()still holds its 2 records afterdeleteAll(), leaving the R1 behavioral contract ("deleteAll MUST NOT touch the captor") without a regression guard.Verification (Rule 2): Read lines 275–284 at HEAD. Confirmed:
repository.save(newPending())× 2,repository.deleteAll(), then only afindByStatusOrderByCreatedAtAsc/getTotalElements().isZero()assertion — no captor assertion present.captorfield confirmed at line 25, initialized in@BeforeEach setUp()at line 30, fully in scope. Finding stands.Disposition: 1 agreed finding kept. No unique-to-one findings to adjudicate. No blockers or majors — verdict CONDITIONAL_APPROVE.
Blast Radius
The diff is entirely contained within the
outbox-test-fixturesmodule — a test-support library. The only production-scope change isOutboxTestAutoConfiguration(matchIfMissingfix). No cross-module contracts are altered and no migration files are touched.BLAST_SCORE: 2/10
CI status (head
3d32f01ab22f)Overall: ✗ failure
2 checks: 2 pending
Findings (1)
[MINOR]
deleteAllClearsBackingStoremissing captor-not-cleared assertion — R1 fix has no regression guardoutbox-test-fixtures/src/test/java/com/aim2be/platform/test/InMemoryOutboxRecordRepositoryTest.java:275
Both reviewers independently identified this gap. The test at line 275 saves 2 records, calls
deleteAll(), then asserts only that the backing store is empty (lines 281–283). It never asserts thatcaptor.listEmitted()still holds 2 entries, so a future change that accidentally addscaptor.clear()insidedeleteAll()would pass all tests.The R1 fix explicitly documents in
InMemoryOutboxRecordRepositorythatdeleteAll()must NOT touch the captor. That behavioral guarantee needs a regression guard here.Add one assertion immediately after line 283:
captoris already a field (line 25) initialized in@BeforeEach setUp()(line 30); no setup change is required.Verdict
CONDITIONAL_APPROVE
hib-pr-reviewer • round 3 • 1 finding (1m) • 2026-05-27T03:30:44.255Z → 2026-05-27T03:31:56.976Z • posted-as: pr-reviewer-bot
Show previous round
hib-pr-reviewer review — PR #5 (affinity-intelligence-rework/im2be-platform-libs)
Round 4 — head
124a820c8baf, basemain, triggersynchronizeTL;DR: CONDITIONAL_APPROVE — verified both unique-to-one findings (A line 61: missing repository.deleteAll() in shared context; B line 56: string-based proxy detection); kept both as minor; all 3 prior-round findings confirmed resolved.
Summary
Arbitration Round 4
Memora context: No prior run history found (both
tags_anyandtags_allqueries returned empty); Memoramemory_createcalls failed with a server-side tag allowlist error ('memora/issues' not in allowed tag list) on every variant attempted — Memora persistence skipped.Reconciliation: Both A and B report 0 agreed findings — each submitted exactly 1 unique finding against the same file (
AutoConfigureOutboxIT.java) at different lines. Both were verified viaReadagainst the actual file at HEAD.Finding A (line 61) — verified ✓: Line 61 is
captor.clear(). Confirmed:saveOnRepositoryRecordsToCaptorclears the captor but never callsrepository.deleteAll(). After this test runs the shared Spring context repository retains the PENDING record saved at line 64. Current tests do not read repository state so nothing fails today, but any future test in this class queryingcountByStatus,findByStatusOrderByCreatedAtAsc, orfindOldestCreatedAtByStatuswill observe contaminated state. Kept.Finding B (line 56) — verified ✓: Line 56 is
assertThat(repository.getClass().getName()).contains("Proxy"). Confirmed: this is a string-based proxy detector. B's analysis is correct —Proxy.isProxyClass(repository.getClass())is the precise JDK predicate for JDK dynamic proxies and would reject CGLIB/ByteBuddy proxies; the string match could pass on unrelated proxy implementations. Kept.All 3 prior-round findings (R1
deleteAll()Javadoc, R1 thread-safety Javadoc, R3 captor-retention assertion) confirmed resolved by both reviewers independently. Not re-raised.Outcome: 2 kept (both minor), 0 dropped. CONDITIONAL_APPROVE.
Blast Radius
Diff is contained entirely within the
outbox-test-fixturesmodule — test-scope infrastructure only. The newInMemoryOutboxRecordRepositoryandOutboxTestAutoConfigurationare consumed by downstream service tests but do not touch any production code path or exported protocol contract.BLAST_SCORE: 2/10
CI status (head
124a820c8baf)Overall: ✗ failure
2 checks: 2 pending
Findings (2)
[MINOR] saveOnRepositoryRecordsToCaptor resets captor but not repository — shared context left with stale PENDING row
outbox-test-fixtures/src/test/java/com/aim2be/platform/test/AutoConfigureOutboxIT.java:61
Line 61 calls
captor.clear()before saving, but there is no correspondingrepository.deleteAll()call. AftersaveOnRepositoryRecordsToCaptorruns, the shared Spring-context repository still holds theOutboxRecordsaved at line 64. The other two tests (captorBeanIsInjectable,repositoryBeanIsInMemory) never read repository state, so nothing fails today. However, any future test added to this class that callscountByStatus(PENDING),findByStatusOrderByCreatedAtAsc(...), orfindOldestCreatedAtByStatus(...)will observe an extra unexpected row, producing wrong counts rather than a clear assertion failure.Add a
@BeforeEachthat resets both the repository and the captor:With this in place the individual
captor.clear()at line 61 becomes redundant and can be removed.[MINOR]
contains("Proxy")is an unreliable proxy-type detector — useProxy.isProxyClass()insteadoutbox-test-fixtures/src/test/java/com/aim2be/platform/test/AutoConfigureOutboxIT.java:56
Line 56 asserts
repository.getClass().getName().contains("Proxy")to verify the in-memory shim was wired. Two problems:InMemoryOutboxRecordRepository.create()ever moves fromProxy.newProxyInstance()to a concrete class or CGLIB subclass, the assertion silently passes (or fails) for the wrong reason.The JDK ships the exact predicate for this use case:
Proxy.isProxyClass()returnstrueiff the class was generated byProxy.newProxyInstance()/getProxyClass(), correctly identifying the shim and rejecting every other proxy mechanism.Verdict
CONDITIONAL_APPROVE
hib-pr-reviewer • round 4 • 2 findings (2m) • 2026-05-27T03:36:42.298Z → 2026-05-27T03:38:11.483Z • posted-as: pr-reviewer-bot
hib-pr-reviewer review — PR #5 (affinity-intelligence-rework/im2be-platform-libs)
Round 5 — head
c7472b39dc8a, basemain, triggersynchronizeTL;DR: CONDITIONAL_APPROVE — 1 agreed minor Javadoc inaccuracy kept (ProxyFactory vs JDK Proxy); all 5 prior-round findings confirmed resolved.
Summary
Round 5 Arbitration Summary
Recalled Memora context: no prior memories found for this PR or submodule. Persistence attempted (tag allowlist errors blocked create; absorb blocked by permissions — best-effort).
Reconciliation: A=1 finding, B=1 finding — both independently flag the identical issue at
OutboxTestAutoConfiguration.java:78: the@returnJavadoc links toorg.springframework.aop.framework.ProxyFactorywhile the actual implementation at line 85 callsInMemoryOutboxRecordRepository.create(captor)which usesjava.lang.reflect.Proxy.newProxyInstance(JDK dynamic proxy). Direct verification confirmed: line 78 reads{@link org.springframework.aop.framework.ProxyFactory}-style;InMemoryOutboxRecordRepository.java:106confirmsProxy.newProxyInstance(loader, interfaces, handler)— no Spring AOP involved.Rule 1 (agreed findings are gold): kept 1 agreed finding.
All 5 prior-round findings confirmed resolved by both reviewers; no re-raise.
Blast Radius
Diff touches a single submodule (
outbox-test-fixtures) containing test infrastructure only. No production code paths are affected. The only change in non-test scope is a Javadoc inaccuracy inOutboxTestAutoConfiguration.java.BLAST_SCORE: 1/10
CI status (head
c7472b39dc8a)Overall: ✗ failure
2 checks: 2 pending
Findings (1)
[MINOR]
@returnJavadoc links toProxyFactorybut implementation uses JDKProxy.newProxyInstanceoutbox-test-fixtures/src/main/java/com/aim2be/platform/test/OutboxTestAutoConfiguration.java:78
Line 78 reads:
org.springframework.aop.framework.ProxyFactoryis Spring AOP's advisor-chain proxy builder (backed by CGLIB/byte-buddy), which is a completely different mechanism from what is actually used. The implementation at line 85 delegates toInMemoryOutboxRecordRepository.create(captor), which callsjava.lang.reflect.Proxy.newProxyInstance(loader, interfaces, handler)(confirmed atInMemoryOutboxRecordRepository.java:106) — a plain JDK dynamic proxy with a customInvocationHandler. TheAutoConfigureOutboxITalso verifies the JDK-proxy contract viaProxy.isProxyClass()at line 57. Clicking the{@link}reference will mislead maintainers into the Spring AOP stack.Replace with:
Verdict
CONDITIONAL_APPROVE
hib-pr-reviewer • round 5 • 1 finding (1m) • 2026-05-27T03:41:58.645Z → 2026-05-27T03:43:33.120Z • posted-as: pr-reviewer-bot