feat(centrifugo): L0 T0 #6 PR-OPAQUE-2 — /token/centrifugo mint endpoint (192-bit ticket + CRC32 + Redis SET EX 300) #2
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/l0-t0-opaque-2-mint-endpoint"
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?
Header
Project: identity-service • PR #2 (L0 T0 #6 PR-OPAQUE-2) • Run R0 (initial submission)
Date: 2026-05-24
TL;DR
READY_FOR_REVIEW— replaces the PR-OPAQUE-1 501 stub with the full ADR-0002 §3 mint flow: 192-bit-entropy ticket + CRC32 + base64url, Redis SET EX 300, per-user rate-limit (30/min default), full OTel + typed error events. 38/38 tests pass on JDK 17 with zero warnings.Summary
L0 Tranche-0 #6 PR-OPAQUE-2 — the actual mint logic for the Centrifugo opaque-ticket flow locked by ADR-0002 Round 3 (Memora #3082).
Scope of this round (initial submission):
TicketGenerator(24 random bytes fromSecureRandom+ 4 CRC32 big-endian bytes → 38 base64url-no-padding ASCII chars).TicketStore(Redis SET EX 300 forcentrifugo:ticket:<value>+ INCR/EX 60 forcentrifugo:mint-rate:<user_id>with configurable cap).CentrifugoTokenControllermint flow: pull verified user from Spring Security context → increment+check rate limit → mint → persist → return{ticket, expires_in: 300}.MintMetrics(countercentrifugo_tickets_minted_total{outcome=...}) + OTel spanticket.mintwithoutcome,user_id_hash,ticket.ttl_seconds,ticket.lengthattributes.TicketMintRateLimitedException(typedIdentityError,ERROR_CODE=TICKET_MINT_RATE_LIMITED) wired toGlobalExceptionHandler→ HTTP 429 +Retry-After: 60.aim2be.centrifugo.mint-rate-limit-per-minute(default 30) declared inapplication.properties.Out of scope (later PRs):
feat/l0-t0-opaque-3-validate-rpc)Commits in this round (each one builds + tests clean):
e889514feat(ticket): TicketGenerator helper + Ticket value objectf622601feat(ticket): TicketStore Redis SET + INCR helper + unit testsb99aed9feat(centrifugo): wire /token/centrifugo mint endpoint with OTelca5c162chore(properties): expose mint rate-limit knob explicitlyNote on commit split: the brief suggested splitting controller (commit 3) and OTel wiring (commit 4); I consolidated into one commit because the observability is integral to the endpoint (the outcome attribute is set INSIDE the controller body, not bolted on afterward — splitting would commit a controller without observability for one revision). Per rule 5 "one concern per commit" reading: the mint endpoint and its observability ARE one concern.
Findings
No new findings this round — initial submission.
Pending follow-ups noted in code comments (NOT findings against this PR):
{user_id, minted_at}today because the JwtAuthenticationFilter only attaches thesubclaim to the security context. PR-OPAQUE-4 will extend SecurityContext to propagatefamily_id,roles,subscription_tier,parental_stateso the validator-side payload is complete.TicketStorejavadoc vs token-bucket / sliding-window. If the SLO numbers from FINDING 53 demand a tighter algorithm we'll iterate.Blast Radius
Score: 3/10
Files touched:
src/main/java/com/aim2be/identity/ticket/{Ticket,TicketGenerator,TicketStore}.java(new)src/main/java/com/aim2be/identity/api/{CentrifugoTokenController,MintMetrics}.java(modified controller from 501 stub; new MintMetrics)src/main/java/com/aim2be/identity/exception/{TicketMintRateLimitedException,GlobalExceptionHandler}.java(new exception + 429 handler)src/main/resources/application.properties(new operator knob)src/test/java/com/aim2be/identity/unit/{ticket,api}/*.java(new tests)Cross-repo impact: zero — identity-service is the verifier-only consumer; no other repo depends on the mint endpoint yet (Centrifugo + realtime-service callers land in PR-OPAQUE-3/5). PR-OPAQUE-3 (sibling worktree at
feat/l0-t0-opaque-3-validate-rpc) reads from the same Redis key prefix (centrifugo:ticket:) — coordinated in the sharedTICKET_KEY_PREFIXconstant.Risk Indicators
SecureRandomallocation (one-shot, JDK-documented thread-safe)/token/centrifugoshifts from 501 to 200/429/503 per ADR-0002 §3 wire format (operator-expected)Verdict
READY_FOR_REVIEW— initial submission. All discipline rules satisfied:mvn installexits 0 with zero[WARNING]lines (verified on JDK 17 locally).RedisConfigwas inspected — defaults (max-active=16) sufficient for the per-mint single round-trip; PR-OPAQUE-3's read path adds the same connection budget pressure but stays within the configured ceiling.:active+:waitingqueues both empty pre-push.Footer
identity-service • mode=manual • run=R0 • tally(resolved=0/new=0/carried=0) • 2026-05-24
Persists minted Centrifugo tickets and enforces per-user mint rate limits via two Redis key spaces (ADR-0002 §3): - centrifugo🎫<base64url> — SET ... EX <ttl> single-round-trip write so a Redis crash between SET and EXPIRE can't leave a TTL-less ticket. PR-OPAQUE-3 reads the same key on the Validate path. - centrifugo:mint-rate:<user_id> — INCR + EX 60 on first hit. Naive fixed-window counter; documented trade-off vs token-bucket. Cap configurable via aim2be.centrifugo.mint-rate-limit-per-minute (default 30). Counter > cap → RateLimitResult.allowed=false; the controller (next commit) returns HTTP 429. Error contract: - RedisConnectionFailureException → TicketMintFailedException (typed, recoverable=true) so GlobalExceptionHandler returns 503 + Retry-After:1. - Unexpected RuntimeException → same wrap. - Null/blank arguments → IllegalArgumentException (programmer error, caller bug). - Null increment return → TicketMintFailedException (Redis contract violation). Logs never include the user_id raw or the JSON payload (rule 01 — secrets / PII). The class name of the underlying exception is the maximum diagnostic surfaced — full cause chain is preserved through exception chaining for the GlobalExceptionHandler / OtelErrorEvents boundary. Tests (17 cases, Mockito-mocked StringRedisTemplate): - store() writes correct prefix + value + TTL - store() wraps Redis connection + unexpected errors - store() rejects null ticket / payload / non-positive TTL - incrementAndCheck() allows / rejects across the boundary value (30 vs 31) - incrementAndCheck() sets EXPIRE only on first hit (count==1) - incrementAndCheck() wraps Redis failures - incrementAndCheck() handles null INCR return - incrementAndCheck() rejects null / blank userId - getMintRateLimitPerMinute() reflects constructor argument Verified clean on JDK 17 (mvn test -Dtest='TicketGeneratorTest,TicketStoreTest'): 26/26 pass, zero compiler warnings.Replaces the PR-OPAQUE-1 501 stub with the full mint flow per ADR-0002 §3: 1. Pull verified user id from Spring Security context (populated by JwtAuthenticationFilter from the verified user JWT's sub claim). 2. Increment + check per-user mint counter via TicketStore — reject with HTTP 429 + Retry-After: 60 when the cap is exceeded. 3. Mint 192-bit-entropy ticket (TicketGenerator) + persist to Redis with 5-minute TTL (TicketStore.store). User context payload: {user_id, minted_at}. Future fields (family_id, roles, subscription tier, parental_state) deferred to PR-OPAQUE-4+ — JwtAuthenticationFilter only attaches the sub claim today. 4. Return {ticket, expires_in: 300} as JSON per ADR-0002 §3. Observability (OTel — combined with the endpoint per rule-5 "one concern per commit" reading: the endpoint and its observability ARE one concern; splitting would land a controller without observability for one commit): - Span ticket.mint (SERVER kind) with attributes: - outcome: success | rate_limited | redis_failure | internal_error - user_id_hash: 16-hex-char SHA-256 prefix (gated by otel.attributes.user-id-hash-enabled GDPR kill-switch — already declared in application.properties from PR-OPAQUE-1) - ticket.ttl_seconds + ticket.length on success - Counter centrifugo_tickets_minted_total{outcome=...} via new MintMetrics helper (mirrors OtelErrorEvents singleton + lazy-init pattern so the counter binds to the post-startup OTel SDK, not the no-op meter present at class-load). - Error counter identity_errors_total bumped via OtelErrorEvents.record in GlobalExceptionHandler for typed failures (TicketMintFailed, TicketMintRateLimited). New exception type TicketMintRateLimitedException (IdentityError, recoverable=true, ERROR_CODE=TICKET_MINT_RATE_LIMITED) carries the counter snapshot (current, limit) for OTel correlation. GlobalExceptionHandler maps it to HTTP 429 + Retry-After: 60 with RFC 9457 problem-details body. Counter values are NOT echoed to the client response — leaking them would reveal abuse-detection state. Logs use OtelErrorEvents.hashUserId; raw user_id never reaches logs (rule 01 — secrets / PII). Ticket value never logged (it's a bearer credential). Tests (11 cases, standalone MockMvc, Mockito-mocked TicketStore): - 200 OK happy path returns ticket + expires_in: 300 - Ticket is exactly 38 base64url chars (ADR-0002 contract) - 429 with Retry-After: 60 on rate-limit rejection - 503 with Retry-After: 1 on Redis failure (both incrementAndCheck + store boundary) - Persisted user context is valid JSON with user_id + minted_at - TTL passed to store is exactly 5 minutes - Boundary rate-limit (count==limit) is allowed - 500 when no authenticated principal (defense-in-depth — should never reach the controller in production, SecurityConfig enforces auth) - Concurrent mint smoke: 10 threads × 100 mints = 1000 unique tickets (uses INHERITABLETHREADLOCAL SecurityContextHolder strategy in test setup so worker threads see the principal) - Rate-limit rejection at first attempt (no stateful local cache) Verified clean on JDK 17 (mvn install -DskipTests + mvn test): - compile: 0 warnings, 0 errors - test: 38/38 pass (9 TicketGenerator + 17 TicketStore + 11 controller + 1 context-loads smoke)Adds aim2be.centrifugo.mint-rate-limit-per-minute=${AIM2BE_...:30} so operators see the knob when grepping application.properties without having to spelunk into TicketStore's @Value default. Matches the sibling-service consistency pattern (rule 10) used for jwt.secret / aim2be.cors.allowed-origins / IDENTITY_REDIS_POOL_*. Default 30 mints/user/minute holds well below the FINDING 53 burst budget (2,000 RPS aggregate) while leaving room for legit reconnect storms — a single user reconnecting once per minute under flaky network conditions is far below the cap. The TicketStore @Value("${...:30}") fallback keeps the runtime default even if the property line is removed, so this commit is purely operator-discoverability + no behaviour change. Verified: mvn install still passes (38/38 tests, 0 warnings).Show previous round
hib-pr-reviewer review — PR #2 (affinity-intelligence-rework/im2be-identity-service)
Round 1 — head
ca5c162a11a8, basemain, triggeropenedTL;DR: NEEDS_WORK — kept 5 findings (2 major, 2 minor, 1 info), all verified via direct file reads; no prior runs for this PR.
Summary
Reconciliation — Round 1
No prior runs found in Memora for this PR. All 5 findings are unique-to-one-reviewer; each was verified via direct
Readof the cited file.Verification results
A1 (MAJOR, kept) —
TicketStore.javalines 182–193:try { redisTemplate.expire(...) } catch (RedisConnectionFailureException ex) { ... }— confirmed. The closing brace is at line 193 with nocatch (RuntimeException)arm. The INCR block (lines 166–174) and thestore()block (lines 123–135) both carry the second arm. Gap is real.B1 (MAJOR, kept) —
CentrifugoTokenController.javalines 157–216: the outercatchclauses (lines 201–213) callspan.setAttribute()via the localspanreference but never callspan.setStatus(StatusCode.ERROR, …). Per JLS §14.20.3.2, theScope(try-with-resources) is closed before the outercatchclauses run, so by the timeGlobalExceptionHandler.handleTicketMintFailed/handleRateLimitedexecute, the span is already ended andSpan.current()is the parent HTTP span. Theticket.mintspan ends withStatusCode.UNSETon all error paths, violating the ADR-0013 §3 tail-sampler contract stated inOtelErrorEventsJavadoc.A2 (MINOR, kept) —
CentrifugoTokenController.javaline 169incrementAndCheckprecedes line 183store.store(). Ifstore()throwsTicketMintFailedException, the counter is committed without a ticket issued. Verified; design trade-off, but undocumented.B2 (MINOR, kept) —
GlobalExceptionHandler.javaline 70:OtelErrorEvents.record(Span.current(), ex, "GlobalExceptionHandler", "redis")insidehandleRateLimitedforTicketMintRateLimitedException. Confirmed — a 429 means Redis returned a counter value above the cap (Redis success). The sibling handler at line 46 uses "redis" correctly. This mislabels burst traffic as Redis errors.B3 (INFO, kept) —
CentrifugoTokenController.javaline 86:private static final String COMPONENT = "CentrifugoTokenController";— confirmed unused throughout the 276-line file.Kept 5, dropped 0.
Blast Radius
The diff touches 11 files spanning the public mint API surface (CentrifugoTokenController), ticket domain (TicketGenerator, TicketStore, Ticket), cross-cutting exception handling (GlobalExceptionHandler, two new typed exceptions), observability (MintMetrics), and three new test suites. GlobalExceptionHandler changes affect all current and future identity-service exception paths, not only the mint endpoint.
BLAST_SCORE: 6/10
Risk Indicators
TicketStore.incrementAndCheck,TicketStore.store,CentrifugoTokenController.mintCentrifugoTicket,CentrifugoTokenController.requireAuthenticatedUserId,GlobalExceptionHandler.handleRateLimited,GlobalExceptionHandler.handleTicketMintFailedCI status (head
ca5c162a11a8)No CI checks reported for this commit.
Related PRs
Findings (5)
[MAJOR] EXPIRE catch block missing
catch (RuntimeException)arm — non-connection Redis errors escape as 500 instead of 503src/main/java/com/aim2be/identity/ticket/TicketStore.java:190
The
try/catchblock guardingredisTemplate.expire(key, RATE_LIMIT_WINDOW)(lines 182–193) catches onlyRedisConnectionFailureExceptionand closes at line 193 with no second arm. Every other Redis I/O boundary in the same class has a pairedcatch (RuntimeException ex)that wraps unexpected errors (e.g.RedisSystemException,QueryTimeoutException,SerializationException) intoTicketMintFailedException:catch (RuntimeException ex) { … throw new TicketMintFailedException(…) }store()block, lines 132–135: same pattern.Without this arm, a non-connection Redis error from
expire()escapes untyped, propagates throughincrementAndCheck(), is caught by the controller'scatch (RuntimeException ex)at line 209 (setsoutcome=internal_error), and is rethrown toGlobalExceptionHandler.handleUntyped()which returns HTTP 500. Clients do not receive the documented 503 +Retry-After: 1, andredis_failureaccounting is lost.Fix — add immediately after line 193:
[MAJOR]
ticket.mintspan status is never set toStatusCode.ERRORon any failure pathsrc/main/java/com/aim2be/identity/api/CentrifugoTokenController.java:201
The mint method uses an extended try-with-resources (try-with-resources + outer catch/finally). Per JLS §14.20.3.2 the
Scoperesource is closed in the inner synthetic finally before the outercatchclauses run. By the timecatch (TicketMintFailedException mintFailed)(line 204) orcatch (RuntimeException ex)(line 209) execute,Span.current()has reverted to the parent HTTP span.The outer catch blocks correctly use the local
spanvariable to callspan.setAttribute(ATTR_OUTCOME, outcome)— those attribute writes land on the right span. However, neither catch block nor thefinallyblock ever callsspan.setStatus(StatusCode.ERROR, …). The span is ended at line 215 (span.end()) withStatusCode.UNSET.When exceptions subsequently reach
GlobalExceptionHandler,OtelErrorEvents.record(Span.current(), …)targets the already-ended parent HTTP span — not theticket.mintspan. The ADR-0013 §3 contract documented inOtelErrorEventsJavadoc line 29 ("Span status set toStatusCode.ERRORso tail-based samplers always retain error traces") is violated for every failingticket.mintspan.Fix — add explicit status calls in the catch blocks where
spanis in scope:[MINOR] Rate-limit slot consumed before
store.store()succeeds — retrying a 503 drains the mint budgetsrc/main/java/com/aim2be/identity/api/CentrifugoTokenController.java:169
store.incrementAndCheck(userId)at line 169 commits the per-user counter increment beforestore.store(ticket, userContextJson, TICKET_TTL)at line 183. Ifstore()throwsTicketMintFailedException(caught at line 204), the slot is consumed but no ticket was issued. The client follows the documentedRetry-After: 1hint and retries; each retry increments the counter again. Under sustained partial Redis degradation (INCR succeeds but SET fails) a user can exhaust all 30 slots per window and begin receiving 429 without ever receiving a usable ticket.The current Javadoc on
incrementAndCheck()documents fail-closed semantics but does not mention this asymmetry. Either:decrementOnFailure(String userId)path called from theTicketMintFailedExceptioncatch block in the controller, or@apiNotetoincrementAndCheck()and the controller documenting this as a deliberate fail-closed invariant so future maintainers do not introduce a compensating decrement without understanding the security implication (uncounted mints).[MINOR]
handleRateLimitedincorrectly labels a 429 as aredisdownstream errorsrc/main/java/com/aim2be/identity/exception/GlobalExceptionHandler.java:70
OtelErrorEvents.record(Span.current(), ex, "GlobalExceptionHandler", "redis")at line 70 passesdownstreamComponent = "redis"for aTicketMintRateLimitedException. A 429 means Redis returned a counter value above the cap — Redis worked correctly. This is not a Redis failure.Labelling rate-limit rejections as a
redisdownstream error will inflateidentity_errors_total{downstream_component="redis"}on normal client burst traffic, potentially triggering false-positive Redis-health alerts.Contrast with
handleTicketMintFailedat line 46, where"redis"is correct because that exception IS caused by a Redis connectivity or command failure.Fix — pass
null(or a purpose-specific label) as the fourth argument:[INFO]
COMPONENTconstant is declared but never usedsrc/main/java/com/aim2be/identity/api/CentrifugoTokenController.java:86
This constant is not referenced anywhere in the 276-line class body. The controller delegates all
OtelErrorEvents.record()calls toGlobalExceptionHandler(which supplies its own component string), and direct span attribute writes in the controller do not use this constant. Either wire it into a futureOtelErrorEventscall in this class or remove it to avoid IDE warnings and dead-code confusion.Verdict
NEEDS_WORK
hib-pr-reviewer • round 1 • 5 findings (2M/2m/1i) • 2026-05-24T02:23:46.258Z → 2026-05-24T02:26:27.178Z • posted-as: pr-reviewer-bot
Show previous round
hib-pr-reviewer review — PR #2 (affinity-intelligence-rework/im2be-identity-service)
Round 2 — head
3693db9cea04, basemain, triggersynchronizeTL;DR: NEEDS_WORK — kept 2 findings (1 major unique-A verified: handleRateLimited marks the HTTP server span ERROR for every 429, violating OTel semconv; 1 minor unique-B verified: serialiseUserContext Jackson failure misrouted to redis_failure/503). All 5 round-1 findings confirmed resolved.
Summary
Arbitration — Round 2
Memora state: No prior run memories found for this PR (Memora persistence attempted but blocked by tag-allowlist constraints; proceeding without persistence).
Verification performed:
GlobalExceptionHandler.java(all 144 lines) — confirmed line 75 callsOtelErrorEvents.record(Span.current(), ex, "GlobalExceptionHandler", "rate-limit").OtelErrorEvents.java(all 412 lines) — confirmed line 172 unconditionally executesspan.setStatus(StatusCode.ERROR, error.getErrorCode())when span != null.CentrifugoTokenController.java(all 296 lines) — confirmed (a) try-with-resourcesScopeat line 161 closes before catch blocks run; (b) rate-limit throw at line 187 propagates through re-throw at line 214 and exits the method entirely before Spring invokeshandleRateLimited; (c) Jackson failure at lines 292–293 is wrapped inTicketMintFailedException; (d) that exception is caught at line 215 withoutcome = OUTCOME_REDIS_FAILURE.Disposition:
@ExceptionHandlermachinery invokeshandleRateLimited, theticket.mintspan'sScopehas already been restored by the try-with-resources exit, soSpan.current()resolves to the OTel auto-instrumented HTTP server span.OtelErrorEvents.recordat line 172 then unconditionally stamps that HTTP server spanERROR, violating OTel HTTP semconv §1.7 (4xx = client error → span status UNSET). Consequence: every 429 inflates HTTP server error-rate dashboards, triggers tail-based-sampler retention on benign rate-limit events, and pollutes theidentity_errors_totalbucket shared with genuine 5xx failures.serialiseUserContextwrapsJsonProcessingExceptioninTicketMintFailedException(lines 292–293), which the controller's catch arm at line 215 stampsoutcome=redis_failureand sets the span to ERROR.GlobalExceptionHandler.handleTicketMintFailedthen returns HTTP 503 +Retry-After: 1. A Jackson schema failure is neither a Redis error nor a retriable condition.Total kept: 2 (1 major + 1 minor). No agreed findings (A and B flagged different issues).
Blast Radius
The diff touches 11 files across four packages (api, exception, ticket, observability) and introduces a new security-critical mint path that issues bearer-credential tickets stored in Redis. The OTel semconv violation in GlobalExceptionHandler is particularly high-blast because it affects all future callers of handleRateLimited, not just the Centrifugo endpoint, and silently corrupts HTTP server error-rate SLOs.
BLAST_SCORE: 5/10
Risk Indicators
mintCentrifugoTicket,TicketStore.incrementAndCheck,TicketStore.store,OtelErrorEvents.hashUserId,requireAuthenticatedUserIdCI status (head
3693db9cea04)No CI checks reported for this commit.
Findings (2)
[MAJOR]
OtelErrorEvents.recordinhandleRateLimitedsetsspan.setStatus(ERROR)on the OTel HTTP server span, violating HTTP semconv for 4xx client errorssrc/main/java/com/aim2be/identity/exception/GlobalExceptionHandler.java:75
OtelErrorEvents.record(Span.current(), ex, "GlobalExceptionHandler", "rate-limit")at line 75 routes throughOtelErrorEvents.java:172, which unconditionally executesspan.setStatus(StatusCode.ERROR, error.getErrorCode())when span is non-null.Why the wrong span is targeted:
mintCentrifugoTicketmanages itsticket.mintchild span viatry (Scope ignored = span.makeCurrent())(line 161). In Java, a try-with-resourcesScopecloses — restoring the parent context — as soon as the try body exits, before any catch block runs. TheTicketMintRateLimitedExceptionthrown at line 187 propagates through the re-throw at line 214 and exitsmintCentrifugoTicketentirely. By the time Spring MVC's exception-resolution infrastructure invokeshandleRateLimited, the entire stack frame ofmintCentrifugoTickethas unwound and theticket.mintScope is closed.Span.current()therefore resolves to the OTel Spring MVC auto-instrumented HTTP server span, not the child span.OTel HTTP semconv violation: OTel HTTP semconv §1.7 requires that HTTP server spans for 4xx responses remain at
StatusCode.UNSET— only 5xx server-side errors should setERROR. The controller code correctly acknowledges this at lines 211–213: "4xx client error per OTel semconv — span stays at StatusCode.UNSET" — buthandleRateLimitedsilently undoes that intent for the parent HTTP server span.Impact: (1) Every 429 response bumps the HTTP server span error rate in Prometheus/Tempo dashboards. (2) Tail-based samplers retain every rate-limit trace as if it were a server error. (3)
identity_errors_total{component=GlobalExceptionHandler, error_type=TicketMintRateLimitedException}is incremented alongside genuine 5xx failures, making the 5xx-only error budget meaningless.Fix: Remove
OtelErrorEvents.record(...)fromhandleRateLimited. The 429 outcome is already fully observable via: (a)centrifugo_tickets_minted_total{outcome="rate_limited"}incremented at line 182; (b)outcome="rate_limited"on theticket.mintchild span; (c)http.response.status_code=429captured automatically by OTel Spring MVC instrumentation. If incrementingidentity_errors_totalis still desired for alerting, callerrorCounter().add(1, ...)directly without touching span status.[MINOR]
serialiseUserContextwrapsJsonProcessingExceptioninTicketMintFailedException, misrouting a programming error tooutcome=redis_failureand HTTP 503src/main/java/com/aim2be/identity/api/CentrifugoTokenController.java:292
At lines 292–293, a
JsonProcessingExceptionfrom Jackson is wrapped and re-thrown asTicketMintFailedException:This is caught by the
catch (TicketMintFailedException mintFailed)arm at line 215, which stamps theticket.mintspan withoutcome=redis_failure, callsspan.setStatus(StatusCode.ERROR, "redis_failure"), and letsGlobalExceptionHandler.handleTicketMintFailedreturn HTTP 503 withRetry-After: 1.Why this is wrong: A Jackson serialisation failure on this fixed two-field payload (
user_id,minted_at) has nothing to do with Redis. Theredis_failurelabel is misleading to on-call engineers. HTTP 503 +Retry-After: 1signals a transient, retriable failure — but retrying will not help if the schema itself is corrupt (e.g., a future field added in a non-serialisable type).The comment at line 290 notes the path is "unreachable in practice" — but if it ever fires, the misattribution creates a confusing on-call incident: Redis dashboards appear healthy while
redis_failurealerts fire.Fix: Change the catch to wrap in
IllegalStateExceptioninstead ofTicketMintFailedException.IllegalStateExceptionis aRuntimeException, so it falls through to the controller'scatch (RuntimeException ex)arm at line 226, which stampsoutcome=internal_error, setsspan.setStatus(StatusCode.ERROR, "internal_error"), and lets the client receive HTTP 500 — the correct signal for an internal programming error:Verdict
NEEDS_WORK
hib-pr-reviewer • round 2 • 2 findings (1M/1m) • 2026-05-24T02:36:15.083Z → 2026-05-24T02:38:51.578Z • posted-as: pr-reviewer-bot
R2 verdict findings (kept=2): (1) MAJOR exception/GlobalExceptionHandler.java:75 — handleRateLimited set HTTP server span ERROR for every 429. Per OTel HTTP semconv §1.7, 4xx responses must keep StatusCode.UNSET; only 5xx errors set ERROR. OtelErrorEvents.record(Span.current(), ...) unconditionally called setStatus(ERROR) on the parent HTTP server span — bumping 4xx-as-server-error count + skewing tail-sampler retention + inflating 5xx error-budget alerts. By the time this handler runs, the controllers ticket.mint child Scope has closed (try-with- resources at controller line 161), so Span.current() resolves to the OTel Spring MVC auto-instrumented HTTP SERVER span — not the intended child. 429 is already fully observable via (a) centrifugo_tickets_minted_total{outcome="rate_limited"}, (b) outcome=rate_limited on the ticket.mint CHILD span, (c) http.response.status_code=429 from OTel Spring MVC auto- instrumentation. Fix: removed OtelErrorEvents.record call entirely. (2) MINOR api/CentrifugoTokenController.java:292 — serialiseUserContext wrapped JsonProcessingException in TicketMintFailedException, misrouting a programming error (someone added a non-serialisable field to the fixed two-field schema) to outcome=redis_failure + HTTP 503 + Retry-After: 1. Retrying wont help if the schema is corrupt; the 503 misleads on-call into checking Redis. Fix: wrap as IllegalStateException so the controllers catch(RuntimeException) arm sets outcome=internal_error + HTTP 500 — the correct signal. Verification: - mvn -DskipTests=false clean install → BUILD SUCCESS - 38 tests pass (CentrifugoTokenControllerTest 11/11, TicketStoreTest 17/17, TicketGeneratorTest 9/9, IdentityServiceApplicationTest 1/1) - Zero warnings on touched modulesShow previous round
hib-pr-reviewer review — PR #2 (affinity-intelligence-rework/im2be-identity-service)
Round 3 — head
02c0a7557bd9, basemain, triggersynchronizeTL;DR: CONDITIONAL_APPROVE — kept 1 minor unique-to-A finding (SpanKind.SERVER → INTERNAL verified at line 158); B submitted 0 formal findings; B's prose-only handleUntyped concern dropped (rule 6 + factually moot under Spring handler resolution).
Summary
Round 3 Reconciliation
Prior-round status: All 7 prior findings (R1: 5 findings; R2: 2 findings) confirmed resolved per both reviewers' summaries.
A's finding (1):
SpanKind.SERVERon manually-createdticket.mintchild span at line 158. Verified —Readat line 158 confirms.setSpanKind(SpanKind.SERVER)is present on thespanBuildercall insidemintCentrifugoTicket(), which already executes within the Spring MVC auto-instrumented HTTP server span. Finding is grounded. Kept.B's prose concern (handleUntyped latent path): Described only in B's prose summary; B's formal
findings[]array was explicitly[]. Per rule 6, prose-only concerns cannot be promoted to structured findings. Additionally, verification ofGlobalExceptionHandler.javaconfirms Spring's@ExceptionHandlerresolution selects the most-specific matching handler —handleRateLimited(typedTicketMintRateLimitedException) is always preferred overhandleUntyped(typedRuntimeException), making the latent-path concern factually moot at the framework level. Dropped.Memora: No prior run memory existed; created new entry (id 216) with
review_count=3covering all three rounds.Result: 1 kept (minor), 0 dropped from A, 0 from B formal findings.
Blast Radius
The finding is confined to the OTel
SpanKindenum value on one manually created span within a single controller method. It has zero functional impact on the mint path, Redis interactions, or error handling — only trace visualization in observability backends is affected. The fix is a one-token change with no ripple across other modules.BLAST_SCORE: 2/10
Risk Indicators
mintCentrifugoTicket,requireAuthenticatedUserId,TicketStore.incrementAndCheck,TicketStore.store,OtelErrorEvents.hashUserIdCI status (head
02c0a7557bd9)No CI checks reported for this commit.
Related PRs
Findings (1)
[MINOR]
SpanKind.SERVERon a manually-created child span violates OTel semantic conventions — should beSpanKind.INTERNALsrc/main/java/com/aim2be/identity/api/CentrifugoTokenController.java:158
Line 158:
.setSpanKind(SpanKind.SERVER)The
ticket.mintspan is created insidemintCentrifugoTicket(), which already executes within the OTel Spring MVC auto-instrumented HTTP server span. Theticket.mintspan therefore becomes a child of that server span.Per the OTel specification §SpanKind,
SERVERindicates server-side handling of a synchronous RPC or remote request — aSERVERspan nested inside anotherSERVERspan is semantically invalid. Practical consequences:tailsamplingprocessor) apply SERVER-span heuristics to what should be an internal span, skewing retention decisions.SERVERspans as service entry points; a nested one produces phantom service nodes or garbled waterfall views.SERVERspan lacks mandatoryhttp.request.method— theticket.mintspan carries none.Fix — change
SpanKind.SERVER→SpanKind.INTERNAL:Verdict
CONDITIONAL_APPROVE
hib-pr-reviewer • round 3 • 1 finding (1m) • 2026-05-24T03:04:16.406Z → 2026-05-24T03:05:55.373Z • posted-as: pr-reviewer-bot
R3 verdict findings (kept=1, all R2 findings resolved → CONDITIONAL_APPROVE): (1) MINOR api/CentrifugoTokenController.java:158 — SpanKind.SERVER on the manually-created `ticket.mint` span violates OTel SpanKind semconv. The span is a CHILD of the OTel Spring MVC auto-instrumented HTTP server span (created inside mintCentrifugoTicket which runs under that server span). Per OTel semconv §SpanKind, SERVER indicates service-entry handling of a synchronous RPC — a nested SERVER span is semantically invalid: (a) tail-based samplers apply SERVER-span heuristics to what should be an internal span, skewing retention decisions (b) Jaeger/Tempo/Grafana Cloud display SERVER spans as service entry points → phantom service nodes / garbled waterfall (c) some exporters warn when a SERVER span lacks http.request.method which this span doesn't carry Fix: SpanKind.SERVER → SpanKind.INTERNAL. Single-token change; zero ripple across other modules (BLAST_SCORE 2/10 per reviewer). Verification: - mvn -DskipTests=false clean install → BUILD SUCCESS - 38/38 tests pass (CentrifugoTokenControllerTest 11/11 + TicketStoreTest 17/17 + TicketGeneratorTest 9/9 + IdentityServiceApplicationTest 1/1) - Zero warnings on touched modulesShow previous round
hib-pr-reviewer review — PR #2 (affinity-intelligence-rework/im2be-identity-service)
Round 4 — head
87976e71e15b, basemain, triggersynchronizeTL;DR: CONDITIONAL_APPROVE — kept 1 minor finding unique to B (SecurityContextHolder strategy scoped too broadly in test setUp); all 8 prior R1–R3 findings confirmed closed by both reviewers.
Summary
Arbitration — Round 4
Memora lookup: No prior run history found in the store (tags unavailable / permissions denied on absorb); treating this as a first-write but the round counter is 4 per the harness.
A=0 B=1: Reviewer A confirmed all 8 prior findings (R1–R3) are resolved and raised nothing new. Reviewer B confirmed the same resolutions and raised one new minor finding unique to B.
Verification of B's unique finding (Rule 2):
Read
CentrifugoTokenControllerTest.javalines 70–94 and grepped forconcurrent. Confirmed:SecurityContextHolder.setStrategyName(SecurityContextHolder.MODE_INHERITABLETHREADLOCAL)is called inside@BeforeEach setUp(), so it fires before every test in the class.@AfterEach tearDown()does restoreMODE_THREADLOCAL, bounding the mutation window to each individual test.concurrentMintsProduceAllUniqueTicketsis the sole test that spawns worker threads and actually requires child-thread context inheritance.parallel=classes/maxParallelForks > 1.Finding verified and kept at minor severity.
Outcome: Kept 1 (unique-to-B, verified); dropped 0. All 8 prior-round findings confirmed closed.
Blast Radius
The diff spans 11 files across three packages (api, ticket, exception) plus test coverage for each. The controller is a live HTTP endpoint, TicketStore owns both the Redis rate-limit counter and ticket persistence, and MintMetrics introduces a new OTel counter surface. Any regression in the rate-limit or store path directly affects all authenticated users requesting Centrifugo tokens.
BLAST_SCORE: 6/10
Risk Indicators
requireAuthenticatedUserId,TicketStore.incrementAndCheck,TicketStore.store,OtelErrorEvents.hashUserIdCI status (head
87976e71e15b)No CI checks reported for this commit.
Findings (1)
[MINOR]
SecurityContextHolder.setStrategyName(MODE_INHERITABLETHREADLOCAL)scoped too broadly — parallel class-level test execution risksrc/test/java/com/aim2be/identity/unit/api/CentrifugoTokenControllerTest.java:76
Line 76 sets the global
SecurityContextHolderstrategy toMODE_INHERITABLETHREADLOCALinside@BeforeEach setUp(), which means every test in this class mutates a JVM-wide static during its window, not just the one concurrent test that actually requires child-thread context inheritance.tearDown()(lines 90–93) does restoreMODE_THREADLOCALafter each test, so the window is bounded — but if the build runs test classes in parallel (Maven Surefireparallel=classes, GradlemaxParallelForks > 1, or JUnit 5junit.jupiter.execution.parallel.enabled=true), another test class executing concurrently could transiently observeMODE_INHERITABLETHREADLOCALinstead of its expectedMODE_THREADLOCAL, producing spurious authentication failures or silent security-context bleed into worker threads.Fix: Remove
setStrategyNamefromsetUp()/tearDown()and scope it only toconcurrentMintsProduceAllUniqueTickets(line 210):The 10 single-threaded tests don't need the inherited strategy — MockMvc dispatches synchronously in the test thread where
MODE_THREADLOCALis already visible.Verdict
CONDITIONAL_APPROVE
hib-pr-reviewer • round 4 • 1 finding (1m) • 2026-05-24T03:12:42.290Z → 2026-05-24T03:14:13.152Z • posted-as: pr-reviewer-bot
R4 verdict findings (kept=1, all 8 prior R1-R3 closed → CONDITIONAL_APPROVE): (1) MINOR test/api/CentrifugoTokenControllerTest:76 — SecurityContextHolder.setStrategyName(MODE_INHERITABLETHREADLOCAL) in @BeforeEach setUp() is scoped TOO BROADLY: it mutates a JVM-wide static for every test in the class, not just the one concurrent test that needs child-thread context inheritance. tearDown() resets it after each test so the window is bounded, BUT under Surefire parallel=classes / Gradle maxParallelForks>1 / JUnit 5 parallel.enabled=true another test class executing concurrently could observe MODE_INHERITABLETHREADLOCAL transiently — producing spurious auth failures OR silent security-context bleed into worker threads of unrelated classes. Fix: removed setStrategyName from setUp + tearDown; moved the strategy mutation into the single concurrent test (concurrentMintsProduceAllUniqueTickets) in a try/finally so the JVM- wide window is one test long, not 10. The strategy switch invalidates the auth set in setUp (different ThreadLocal store), so the concurrent test re-sets the authentication after the switch. Split the body into the @Test wrapper + a private runConcurrentMintsTest helper so the try/finally stays readable. Verification: - mvn -DskipTests=false clean install → BUILD SUCCESS - 38/38 tests pass (CentrifugoTokenControllerTest 11/11 including the scope-limited concurrent test; rest unchanged) - Zero warnings on touched modulesShow previous round
hib-pr-reviewer review — PR #2 (affinity-intelligence-rework/im2be-identity-service)
Round 5 — head
97b6222f8c3b, basemain, triggersynchronizeTL;DR: CONDITIONAL_APPROVE — kept all 4 unique-to-one findings after individual verification (2 minor, 2 info); no blocking or major issues; all 9 prior-round findings confirmed resolved.
Summary
Round 5 Arbitration — 4 findings kept (all unique-to-one, all verified)
Memora recall
No prior Memora memory found for this PR despite being round 5 (store permission issues prevented write). Treated as first-record; findings reconciled from scratch against source.
Prior-round resolutions
Both reviewers independently confirm all 9 prior-round findings (R1–R4) are resolved at HEAD. Not re-raised.
Unique-to-A findings (3) — all verified
MintMetrics.java:57:resetForTesting()is package-private incom.aim2be.identity.api; all test classes are incom.aim2be.identity.unit.*(confirmed viagrep ^packageacross all 4 test files). Method is genuinely unreachable dead code today.Ticket.java:29–33: compact constructor confirmed to check only null/empty; no length guard againstTicketGenerator.EXPECTED_BASE64URL_LENGTH=38. Code matches the finding exactly.GlobalExceptionHandler.java:.header("Retry-After", "60")confirmed at line 97 (reviewer cited 95 — the start of the builder chain; corrected to 97 in the finding). Full-window hardcode verified.Unique-to-B findings (1) — verified
TicketStoreTest.java:182–188: the existing test mocksvalueOperations.increment()to throw — it exercises the INCR catch arm only.TicketStore.javalines 190–201 have two distinct catch arms onredisTemplate.expire()that have zero test coverage. Confirmed by reading both files in full.All 4 findings are code-grounded. No findings dropped.
Blast Radius
The diff introduces the full mint path across 7 new/modified production classes (controller, metrics, store, generator, value object, exception types, global handler) plus 3 new test files. The surface is self-contained within the Centrifugo ticket submodule but the
GlobalExceptionHandleris a shared boundary that affects all identity-service endpoints, andTicketStoreholds the Redis rate-limit and persistence logic that gates every mint request.BLAST_SCORE: 6/10
Risk Indicators
TicketStore.incrementAndCheck,CentrifugoTokenController.mintCentrifugoTicket,GlobalExceptionHandler.handleRateLimited,TicketStore.storeCI status (head
97b6222f8c3b)No CI checks reported for this commit.
Findings (4)
[MINOR]
resetForTesting()is unreachable dead code — no test class can call itsrc/main/java/com/aim2be/identity/api/MintMetrics.java:57
Verified.
MintMetricsis in packagecom.aim2be.identity.api(line 1 of the file). The method is package-private (no modifier):All four test classes in this PR confirmed to live in
com.aim2be.identity.unit.*packages:CentrifugoTokenControllerTest→com.aim2be.identity.unit.apiTicketStoreTest→com.aim2be.identity.unit.ticketTicketGeneratorTest→com.aim2be.identity.unit.ticketIdentityServiceApplicationTest→com.aim2be.identity.unitNone are in
com.aim2be.identity.api, so Java package-private visibility makesresetForTesting()unreachable. The singleton is never reset between tests; the OTel no-op counter created at first class-load survives the entire Surefire JVM. This is harmless for current tests but will silently sabotage any futureMintMetrics-aware assertion test.The method also introduces a static mutator into production code with no production caller — a future refactor adding an in-package caller would reset a live counter.
Fix (pick one): delete the method (use
InMemoryMetricReaderfor counter assertions); or move aMintMetricsTestHelpershim intosrc/test/java/com/aim2be/identity/api/so the method is legally reachable.[INFO] Compact constructor validates null/empty but not the ADR-0002 §3 fixed-38-char length contract
src/main/java/com/aim2be/identity/ticket/Ticket.java:29
Verified. The compact constructor at lines 29–33:
TicketGeneratorpublishesEXPECTED_BASE64URL_LENGTH = 38precisely because the ADR-0002 §3 wire contract is fixed at 38 base64url characters. TheTicketvalue object enforces no such invariant, so a caller that passes a truncated or raw-hex string constructs a validTicketinstance. That value is then stored in Redis and returned to clients; the error surfaces only at the PR-OPAQUE-3 validator side rather than at mint time.Suggested addition:
[INFO]
Retry-After: 60always sends the full window, not the remaining TTL — clients over-backoffsrc/main/java/com/aim2be/identity/exception/GlobalExceptionHandler.java:97
Verified. Line 97:
The
centrifugo:mint-rate:<user_id>counter key is set with a 60-second TTL on first increment. A user who hits the cap at second 55 of the window has only 5 seconds left before the counter rolls — yetRetry-After: 60instructs them to wait the full window again, wasting up to 59 seconds of legitimate mint capacity.TicketStore.incrementAndCheck()already holds the Redis template; aredisTemplate.getExpire(key, TimeUnit.SECONDS)call after the INCR check would letRateLimitResultcarryremainingWindowSeconds, whichGlobalExceptionHandlercould forward verbatim.This is a UX/efficiency concern, not a security or correctness regression — the conservative over-backoff is safe. Worth deciding before PR-OPAQUE-3 exposes this header to mobile clients.
[MINOR] EXPIRE failure paths in
incrementAndCheck()have no unit tests — R1 fix unguarded against regressionsrc/test/java/com/aim2be/identity/unit/ticket/TicketStoreTest.java:188
Verified.
TicketStore.javalines 190–201 (the R1 fix) add two catch arms onredisTemplate.expire():The existing test at lines 182–188 (
incrementAndCheckWrapsRedisConnectionFailure) mocksvalueOperations.increment()to throw — it exercises the INCR catch arm only. There is no test that returns1Lfromincrement()(triggering the EXPIRE branch) and then hasredisTemplate.expire()throw. Both catch arms on the EXPIRE call are untested; a future refactor silently removing them would not be caught.Add two tests after line 188:
Verdict
CONDITIONAL_APPROVE
hib-pr-reviewer • round 5 • 4 findings (2m/2i) • 2026-05-24T03:21:37.247Z → 2026-05-24T03:23:57.014Z • posted-as: pr-reviewer-bot
R5 verdict findings (kept=4 — 2 MINOR + 2 INFO; all 9 prior R1-R4 closed): (1) MINOR api/MintMetrics.java — `resetForTesting()` was unreachable dead code (no test class could call it because all tests use a mocked SDK + the singleton is package-private). Removed. (2) MINOR test/ticket/TicketStoreTest — EXPIRE failure paths in `incrementAndCheck()` had NO test coverage. The R1 catch-arm fix (RedisConnectionFailureException + generic RuntimeException both wrap as TicketMintFailedException) was unguarded against regression. Added 2 new tests exercising both EXPIRE catch arms. (3) INFO ticket/Ticket.java — compact constructor only validated null/empty; did NOT enforce the ADR-0002 §3 fixed-38-char length contract or the base64url alphabet. A mint-side regression (wrong entropy size, standard-base64 encoder swap) would surface only on the Validate side as TICKET_NOT_FOUND. Fixed: length check + base64url-alphabet regex. EXPECTED_LENGTH=38 exposed as a public constant. Added 2 new tests covering wrong-length + non-base64url characters. (4) INFO exception/GlobalExceptionHandler.java — `Retry-After: 60` was a hardcoded full-window value; rate-limited clients over-backed-off when only a few seconds remained in the window. Fixed: TicketStore.incrementAndCheck() now queries Redis PTTL on the rate-limit key when rejecting; threads remainingWindowSeconds through RateLimitResult → TicketMintRateLimitedException → handler's Retry-After header. Best-effort: Redis hiccup during PTTL falls back to the full window (60s) so the 429 still surfaces without losing the rate-limit signal. Added 3 new tests (dynamic Retry-After, fallback on PTTL failure, fallback on no-TTL key). Test count: 38 → 45 (+7 new tests across the 4 fixes). Verification: `mvn -DskipTests=false clean install` → BUILD SUCCESS, 45/45 tests pass, zero warnings.Show previous round
hib-pr-reviewer review — PR #2 (affinity-intelligence-rework/im2be-identity-service)
Round 6 — head
7a463e12011d, basemain, triggersynchronizeTL;DR: CONDITIONAL_APPROVE — kept 2 minor doc-only findings (1 GOLD agreed, 1 unique-to-B verified); all 13 prior-round functional issues confirmed resolved; no blocking concerns.
Summary
Round 6 Arbitration — 2 findings kept
Prior findings (R1–R5, 13 total): Both reviewers independently confirmed all resolved. No re-raise.
Finding 1 — GOLD (A + B agreed):
TicketMintRateLimitedException.javaclass Javadoc at line 11 still hardcodesRetry-After: 60. A cited line 13 (info), B cited line 11 (minor). Actual stale text is at line 11 ({@code Retry-After: 60} header — the rate-limit window is 60 seconds). Independent agreement → GOLD; B's line number is more precise. Severity set to minor (higher of the two, grounded by B's reasoning that it misleads integrators implementing retry logic).Finding 2 — unique-to-B, verified:
CentrifugoTokenController.javamethod Javadoc at line 146 still reads{@code Retry-After: 60}. B cited line 149; actual stale text is at line 146 (confirmed viaRead). Finding substantively correct; kept with corrected line number.Memora persistence: tool unavailable this session (tag allowlist + permission constraints on
memory_create/memory_absorb); run summary could not be stored.Blast Radius
The diff touches one new controller endpoint, its Redis-backed rate-limit and ticket-store collaborators, associated exception types, and three test suites. Impact is confined to the
/token/centrifugomint path within a single service; no shared library, protocol contract, or migration file is modified. The two remaining findings are Javadoc-only and carry no runtime blast.BLAST_SCORE: 4/10
Risk Indicators
mintCentrifugoTicket,incrementAndCheck,TicketStore.store,getRetryAfterSeconds,requireAuthenticatedUserIdCI status (head
7a463e12011d)No CI checks reported for this commit.
Related PRs
Findings (2)
[MINOR] Class Javadoc still claims
Retry-After: 60afterretryAfterSecondsmade the header dynamicsrc/main/java/com/aim2be/identity/exception/TicketMintRateLimitedException.java:11
Line 11 reads:
The
retryAfterSecondsfield (line 36) introduced in this PR makes the header dynamic —GlobalExceptionHandler.handleRateLimited()now emitsLong.toString(ex.getRetryAfterSeconds()), not the literal"60". A developer or operator reading this class contract will incorrectly assume a fixed 60-second back-off rather than honoring the actual header value (which can be anywhere from 1 to 60 s depending on the Redis-reported remaining TTL).Replace lines 10–12 with:
Source: Both reviewers A (line 13, info) and B (line 11, minor) independently raised this — GOLD finding. Actual stale text is at line 11 (verified via
Read). Severity kept at minor (higher of the two ratings; B's reasoning — misleads callers implementing retry logic — is well-grounded).[MINOR] Method Javadoc still claims
Retry-After: 60after the header became dynamicsrc/main/java/com/aim2be/identity/api/CentrifugoTokenController.java:146
Line 146 reads:
Since the R5 fix landed,
Retry-Afteris no longer a fixed 60; it carries the Redis-reported remaining TTL on the rate-limit key (falling back to 60 only when the TTL fetch fails). A developer reading this error-contract bullet — especially a PR-OPAQUE-3 author integrating client retry logic — will implement a fixed 60 s back-off instead of honouring the actual header value.Replace with:
Source: Unique to reviewer B (cited line 149). Verified via
Read: stale text is at line 146 (B's citation off by 3). Substantively correct; kept with corrected line number.Verdict
CONDITIONAL_APPROVE
hib-pr-reviewer • round 6 • 2 findings (2m) • 2026-05-24T15:14:06.235Z → 2026-05-24T15:16:53.972Z • posted-as: pr-reviewer-bot
R6 verdict findings (kept=2, all doc-only; all 13 prior R1-R5 functional issues resolved): (1) MINOR exception/TicketMintRateLimitedException.java:11 — class Javadoc still described `Retry-After: 60` as fixed after R6 made it dynamic via the new `retryAfterSeconds` field + `getRetryAfterSeconds()` accessor. Updated to describe the dynamic header (Redis-reported remaining TTL with full-window fallback). (2) MINOR api/CentrifugoTokenController.java:146 — class Javadoc error contract block still listed `Retry-After: 60` as the fixed 429 header. Updated to describe the dynamic value + the fallback semantics. Verification: - mvn -DskipTests=false clean install → BUILD SUCCESS - 45/45 tests pass (no test changes; doc-only refresh)Show previous round
hib-pr-reviewer review — PR #2 (affinity-intelligence-rework/im2be-identity-service)
Round 7 — head
6ba9a2b1da68, basemain, triggersynchronizeTL;DR: CONDITIONAL_APPROVE — verified and kept 2 unique findings (minor stale Javadoc, info stale line-number cross-ref); all 15 prior-round findings confirmed fixed.
Summary
Round 7 Arbitration
Memora recalled 0 prior-run memories (no existing entry for this PR). Both reviewers each brought 1 unique finding; neither is agreed. Both were verified via
ReadonGlobalExceptionHandler.javaandCentrifugoTokenController.java.Kept 2 findings (both unique-to-one, both grounded by verification):
Unique-to-B (GEH:57, minor) —
handleRateLimitedJavadoc still reads{@code Retry-After: 60}. Verified at line 57: text confirmed stale. Implementation at line 102 usesLong.toString(ex.getRetryAfterSeconds()). The R6 pass updated the identical stale text inTicketMintRateLimitedException.java:11andCentrifugoTokenController.java:146but bypassed this method's own Javadoc. KEPT.Unique-to-A (GEH:75, info) — Comment says
try-with-resources at line 161. Verified at CentrifugoTokenController.java:170:try (Scope ignored = span.makeCurrent())is there. Line 161 is inside a prose comment block aboutSpanKind.INTERNAL, not a try-statement. Reference is off by 9. KEPT.All 15 prior-round findings (R1–R6) are confirmed fixed at HEAD per both reviewers; none re-raised.
Memora persistence: tag allowlist validation prevented writes; absorb also required permissions not granted. Run summary not persisted.
Blast Radius
Changes are confined to a single exception handler class (GlobalExceptionHandler.java) and its cross-references to CentrifugoTokenController. The defects are documentation-only (stale Javadoc and a wrong comment line number); no behavioural or API contract changes are implied. Blast radius is minimal.
BLAST_SCORE: 2/10
Risk Indicators
handleRateLimited,mintCentrifugoTicket,incrementAndCheckCI status (head
6ba9a2b1da68)No CI checks reported for this commit.
Findings (2)
[MINOR] handleRateLimited Javadoc still claims
Retry-After: 60after implementation became dynamicsrc/main/java/com/aim2be/identity/exception/GlobalExceptionHandler.java:57
Line 57 reads:
The actual implementation at line 102 uses
Long.toString(ex.getRetryAfterSeconds())— the Redis-reported remaining TTL fromRateLimitResult.remainingWindowSeconds(), not the hard-coded 60. The R5 fix changed the implementation; the R6 pass updated the same stale text inTicketMintRateLimitedException.java:11andCentrifugoTokenController.java:146, but this method's own Javadoc was missed.Proposed fix: update lines 57–58 to reflect dynamic behaviour, e.g.:
[INFO] Cross-file line-number reference in handleRateLimited comment is off by 9
src/main/java/com/aim2be/identity/exception/GlobalExceptionHandler.java:75
Line 75 reads:
Verified:
try (Scope ignored = span.makeCurrent())inCentrifugoTokenController.javais at line 170, not line 161. Line 161 in that file falls inside a prose comment block aboutSpanKind.INTERNALsemantics — not a try-statement at all.Proposed fix: change
line 161→line 170, or drop the fragile line-number entirely and writetry-with-resources on the ticket.mint Scopeinstead.Verdict
CONDITIONAL_APPROVE
hib-pr-reviewer • round 7 • 2 findings (1m/1i) • 2026-05-24T15:23:13.260Z → 2026-05-24T15:25:09.827Z • posted-as: pr-reviewer-bot
R7 verdict findings (kept=2, all doc-only; all 15 prior R1-R6 functional + doc issues closed): (1) MINOR exception/GlobalExceptionHandler.java:57-58 — handleRateLimited Javadoc still described `Retry-After: 60` as fixed. R5 made the header dynamic; R6 updated the class Javadoc on TicketMintRateLimitedException + the controller's class Javadoc, but missed this method's own Javadoc. Updated to describe the dynamic header (Redis-reported remaining TTL + full-window fallback semantics + link to getRetryAfterSeconds()). (2) INFO exception/GlobalExceptionHandler.java:75 + 84 — comment block referenced stale line numbers in CentrifugoTokenController. The try-with-resources reference at "line 161" actually fell inside a prose comment block at that line; similarly the "line 182" centrifugo_tickets_minted_total reference was stale. Fix: dropped both fragile line-number cross-references entirely; replaced with symbolic descriptions ("try-with-resources on the ticket.mint Scope inside mintCentrifugoTicket" + "incremented in the controller's mint path"). Future controller refactors no longer create stale references in this handler's comment. Verification: - mvn -DskipTests=false clean install → BUILD SUCCESS - 45/45 tests pass (no test changes; doc + comment refresh only)Show previous round
hib-pr-reviewer review — PR #2 (affinity-intelligence-rework/im2be-identity-service)
Round 8 — head
f9ea810b69ee, basemain, triggersynchronizeTL;DR: CONDITIONAL_APPROVE — kept all 3 findings (1 unique-to-A + 2 unique-to-B), all verified at HEAD; no blocking issues.
Summary
Arbitration Round 8
Memora recall: No prior run history found (tags allowlist prevented persistence; absorbed via fallback — absorb permission not granted either; noted as infrastructure limitation).
Verification performed:
A-unique — TicketStore.java:228 (
PTTLin log vsgetExpirein code): Confirmed byReadat lines 216 and 228.redisTemplate.getExpire(key)maps to RedisTTL(integer seconds); log string says"Rate-limit PTTL fetch failed".PTTLis a distinct Redis command returning milliseconds. Finding is factually correct — kept.B-unique — CentrifugoTokenControllerTest.java:291 (
executor.shutdown()unreachable on timeout): Confirmed byReadlines 291–293.assertTrue(done.await(60, TimeUnit.SECONDS), ...)throwsAssertionErroron latch timeout;executor.shutdown()at line 293 is in straight-line code after it, not in afinally. Leaked threads can race with the outerfinallythat resetsSecurityContextHolderstrategy. Finding is accurate — kept.B-unique — CentrifugoTokenControllerTest.java:254 (comment blames
setUp()for strategy change): Confirmed byReadlines 245 and 254. Strategy changeSecurityContextHolder.setStrategyName(MODE_INHERITABLETHREADLOCAL)is at line 245 insideconcurrentMintsProduceAllUniqueTickets(), notsetUp(). Comment at line 254 still says"The strategy change in setUp()". Finding is accurate — kept (info).No findings overlapped between A and B. All three are unique-to-one, all three verified against HEAD. Kept all 3.
Blast Radius
The diff spans 11 files across controller, ticket subsystem, exception handling, observability, and tests. The new
/token/centrifugoendpoint is an exported HTTP surface consumed by Centrifugo-connected clients. Impact is bounded to the new mint path; no shared infrastructure (auth filter, DB migrations) was modified.BLAST_SCORE: 5/10
Risk Indicators
incrementAndCheck,store,requireAuthenticatedUserId,OtelErrorEvents.hashUserId,TicketGenerator.generateCI status (head
f9ea810b69ee)No CI checks reported for this commit.
Related PRs
Findings (3)
[MINOR] Log message says "PTTL fetch failed" but code calls
getExpire(Redis TTL, not PTTL)src/main/java/com/aim2be/identity/ticket/TicketStore.java:228
Line 228:
redisTemplate.getExpire(key)at line 216 maps to the RedisTTLcommand, which returns the remaining TTL in seconds.PTTLis a separate Redis command returning milliseconds. The log message is factually wrong and will mislead an on-call engineer who typesPTTL centrifugo:mint-rate:<user>in redis-cli expecting to see what the warning described. Fix:The variable name
remainingWindowSecondsat line 204 and all surrounding comments are correct; only this one log string needs updating.[MINOR]
ExecutorServicenot shut down in try/finally — leaked threads on latch timeoutsrc/test/java/com/aim2be/identity/unit/api/CentrifugoTokenControllerTest.java:291
Lines 291–293:
If the 60-second latch times out,
assertTruethrowsAssertionErrorbeforeexecutor.shutdown()is reached. The 10 non-daemon threads remain alive and can race with the outerfinallyblock (lines 248–250 ofconcurrentMintsProduceAllUniqueTickets) that resetsSecurityContextHolderstrategy back toMODE_THREADLOCAL, causing spuriousNullPointerExceptions in unrelated tests.Fix — wrap in a
try/finallyinsiderunConcurrentMintsTest():[INFO]
runConcurrentMintsTestcomment attributes strategy mutation tosetUp()— wrong method after R4 fixsrc/test/java/com/aim2be/identity/unit/api/CentrifugoTokenControllerTest.java:254
Line 254:
As of the R4 fix,
setUp()no longer touches theSecurityContextHolderstrategy at all. The strategy change now happens in the callerconcurrentMintsProduceAllUniqueTickets()at line 245. A future reader following the comment tosetUp()will find nothing there. Update the comment:Verdict
CONDITIONAL_APPROVE
hib-pr-reviewer • round 8 • 3 findings (2m/1i) • 2026-05-24T15:34:51.833Z → 2026-05-24T15:36:41.639Z • posted-as: pr-reviewer-bot
R8 verdict findings (kept=3; all 17 prior R1-R7 issues closed): (1) MINOR ticket/TicketStore.java:228 — log message said "PTTL fetch failed" but `redisTemplate.getExpire(key)` returns the TTL in seconds (TTL command, not PTTL milliseconds). Fix: → "TTL fetch failed" so operator dashboards aren't misled. (2) MINOR test/api/CentrifugoTokenControllerTest:291 — ExecutorService leak risk on latch timeout. `assertTrue(done.await(60s))` would throw AssertionError BEFORE `executor.shutdown()` could fire, leaving 10 non-daemon threads alive to race with the outer SecurityContextHolder strategy reset + produce spurious NPEs in unrelated tests. Fix: wrapped the assertTrue in try/finally around `executor.shutdownNow()` (interrupts stuck threads; idempotent if already done). (3) INFO test/api/CentrifugoTokenControllerTest:254 — `runConcurrentMintsTest` comment attributed strategy mutation to setUp() but R4 moved it OUT of setUp into the wrapper test's try/finally. Fix: rewrote comment to describe the actual semantics (auth in MODE_THREADLOCAL store, wrapper switches strategy, helper re-sets auth in the new ThreadLocal). Verification: - mvn -DskipTests=false clean install → BUILD SUCCESS - 45/45 tests pass (no test added; structural change to concurrent-test cleanup + 2 doc/log fixes)hib-pr-reviewer review — PR #2 (affinity-intelligence-rework/im2be-identity-service)
Round 9 — head
5b7cff0a8bbf, 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 9 review — all 20 prior-round findings resolved; 3 new lower-severity findings
Prior-round closure confirmation (all 20 verified at HEAD)
catch(RuntimeException)ticket.mintspan never setsStatusCode.ERRORstore.store()handleRateLimitedlabels 429 asredisdownstream errorOtelErrorEvents.recordremovedCOMPONENTconstant unusedOtelErrorEvents.recordsetsERRORon 4xx spanserialiseUserContextwraps inTicketMintFailedExceptionIllegalStateExceptionSpanKind.SERVERon child spanSpanKind.INTERNALSecurityContextHolder.setStrategyNamescoped too broadlyresetForTesting()dead codeTicketcompact constructor lacks 38-char length checkRetry-After: 60always full windowgetRetryAfterSeconds()TicketMintRateLimitedExceptionJavadoc claimsRetry-After: 60Retry-After: 60handleRateLimitedJavadoc claimsRetry-After: 60CI status (head
5b7cff0a8bbf)No CI checks reported for this commit.
Findings
No new findings this round.
Quorum converged on empty findings (A + B both returned 0).
Verdict
NO_NEW_FINDINGS
hib-pr-reviewer • round 9 • 0 findings • 2026-05-24T15:38:55.460Z → 2026-05-24T15:48:21.387Z • posted-as: pr-reviewer-bot • [bookkeeping fallback]