feat(proto): L0 T0 #6 PR-OPAQUE-4a — family/v1 + user/v1 mint-side context RPCs #1
Loading…
Add table
Add a link
Reference in a new issue
No description provided.
Delete branch "feat/l0-t0-opaque-4a-family-user-context-protos"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Summary
Adds 2 new proto contracts for the PR-OPAQUE-4 gRPC fan-out flow (option b — operator decision 2026-05-24):
family/v1/family_context.proto→FamilyContextService.GetFamilyForUser(user_id)returnshas_family+family_id+ParentalControlState(4 lock flags).user/v1/user_profile.proto→UserProfileService.GetUserProfile(user_id)returnsSubscriptionTier(FREE/PRO/FAMILY) +is_admin.Both use
shared/v1/ids.protostrong-typed UUID wrappers. ParentalControlState + SubscriptionTier are wire-identical to their realtime/v1 counterparts (consumer-side copies in the Centrifugo Validate response); duplication is intentional so each service owns its source-of-truth contract.PR-OPAQUE-4 mint flow (forthcoming)
Per Stage B Envoy Pattern B (ADR-0008), both RPCs travel through SPIFFE mTLS via each service's Envoy sidecar; SPIRE-enforced ACLs restrict to identity-service today.
Verified
buf lint→ clean (no output, exit 0)buf build→ clean (proto compiles for Java/TS/Python codegen targets per buf.gen.yaml)shared/v1/ids.protoimports resolveUnblocks
Both can land in parallel once this PR merges.
Test plan
buf lintcleanbuf buildcleanPR-OPAQUE-4 prerequisite (option b — gRPC fan-out per operator decision 2026-05-24). Adds 2 service contracts for identity-service to call at /token/centrifugo mint time to aggregate user-context before storing in Redis as the Centrifugo opaque ticket payload. PR-OPAQUE-4 flow (forthcoming): Client → POST /token/centrifugo (Bearer JWT) → identity-service: verify JWT (sub = user_id) identity-service → family-service.GetFamilyForUser(user_id) → user-service.GetUserProfile(user_id) [parallel] Aggregated context → Redis SET EX 300 → ticket → client Contracts: `family/v1/family_context.proto`: - FamilyContextService.GetFamilyForUser(GetFamilyForUserRequest) returns (GetFamilyForUserResponse) - Returns has_family + family_id + ParentalControlState (4 lock flags). Empty-family users (fresh accounts pre-family-creation) get has_family=false + zero-value response; unknown user_id returns NOT_FOUND gRPC status. - family/v1/ParentalControlState is wire-identical to realtime.v1/ParentalControlState (Centrifugo Validate response consumer-side copy). Duplication is intentional: family-service is the source-of-truth; realtime.v1 mirrors for the Centrifugo contract. A future lock-flag change lands HERE first; the realtime/v1 copy updates in lockstep. `user/v1/user_profile.proto`: - UserProfileService.GetUserProfile(GetUserProfileRequest) returns (GetUserProfileResponse) - Returns SubscriptionTier (FREE/PRO/FAMILY) + is_admin. The response is a snapshot at read time; Centrifugo subscribe_proxy re-checks tier at channel-subscribe time per ADR-0002 §4 so a stale snapshot is recoverable. - user/v1/SubscriptionTier is wire-identical to realtime.v1/SubscriptionTier. Duplication same rationale. Both contracts: - Use shared/v1/ids.proto UserId / FamilyId strong-typed wrappers. - Per Stage B Envoy Pattern B (ADR-0008): each service exposes the RPC via its Envoy sidecar with SPIFFE mTLS. SPIRE-enforced ACL restricts identity-service → both RPCs today; future consumers (admin-service, notification-service, etc.) added by ACL update without touching the proto contract. Verified: - `buf lint` → clean - `buf build` → clean (proto compiles for Java/TS/Python codegen targets per buf.gen.yaml) - shared/v1/ids.proto import path resolves - No name conflicts with realtime/v1 (different packages) Unblocks: - PR-OPAQUE-4b (family-service implementation of GetFamilyForUser + gRPC bootstrap) - PR-OPAQUE-4c (user-service implementation of GetUserProfile + gRPC bootstrap) - Both can land in parallel once this PR merges.Show previous round
hib-pr-reviewer review — PR #1 (affinity-intelligence-rework/im2be-protobuf)
Round 1 — head
67e3d08f4809, basemain, triggeropenedTL;DR: CONDITIONAL_APPROVE — kept 4 findings (2 agreed, 2 unique-to-one verified, 0 dropped); no blocking issues.
Summary
Arbitration — Round 1
Memora recall: No prior runs for this PR; no submodule-level patterns stored. First arbitration.
Files read for verification:
family/v1/family_context.proto(111 lines, full)user/v1/user_profile.proto(91 lines, full)realtime/v1/connect.proto(129 lines, full) — to verify cross-file duplication claimsshared/v1/ids.proto— to confirmFamilyIdis a message type (needed for B1 field-presence claim)Reconciliation decisions
A1 + B2 → AGREED (F1): Both reviewers independently flag
user/v1/user_profile.proto:78as a wire-identical but unenforced duplicate ofrealtime.v1.SubscriptionTier. Verified againstrealtime/v1/connect.proto:94–104: ordinals 0–3, same names, same semantics. No import or CI gate enforces parity. Kept as minor.A2 (unique-to-A, B2 partial corroboration) → VERIFIED (F2): A cites
family/v1/family_context.proto:97; B2 explicitly namesfamily/v1/family_context.proto:91–96for the same concern. Verified againstrealtime/v1/connect.proto:107–116:ParentalControlStatehas identical fields 1–4. A correctly notes the higher-consequence failure mode (silent lock bypass = child accesses restricted feature vs. tier downgrade). Kept as minor.B1 (unique-to-B) → VERIFIED (F3):
family/v1/family_context.proto:72isbool has_family = 1;. Line 77 isshared.v1.FamilyId family_id = 2;.FamilyIdis confirmed as a proto3 message type inshared/v1/ids.proto:22–25, giving it inherent field presence. The redundancy is real: a server bug that setshas_family = truebut omitsfamily_idcreates nil-dereference risk at callers that guard on the bool. Kept as minor.B3 (unique-to-B) → VERIFIED (F4):
user/v1/user_profile.proto:81carries the comment "never published; indicates missing entitlement (caller-side bug or DB anomaly)" but does not specify that callers must refuse to mint a ticket (fail-closed) rather than mappingUNSPECIFIEDtoFREE. Proto3 wire default means a serialisation fault and an explicit DB anomaly are indistinguishable at the caller. A one-sentence contract addition closes the gap. Kept as info.Memora persistence: Run summary stored (memory id 218, fallback tags due to allowlist restrictions).
Kept: 4 findings (2 agreed, 2 unique-to-one verified). Dropped: 0.
Blast Radius
The two new proto files define the gRPC fan-out contract at the /token/centrifugo mint boundary, directly affecting identity-service's auth token issuance path and downstream Centrifugo subscribe_proxy feature gating. The SubscriptionTier and ParentalControlState duplication risk also extends to realtime/v1/connect.proto consumers (realtime-service's Validate handler). However, these are net-new files with no existing generated-SDK consumers at merge time, limiting immediate blast radius.
BLAST_SCORE: 5/10
Risk Indicators
GetFamilyForUser,GetUserProfile,SubscriptionTier,ParentalControlStateCI status (head
67e3d08f4809)No CI checks reported for this commit.
Findings (4)
[MINOR]
SubscriptionTierduplicated fromrealtime.v1— no mechanical sync guard, silent ordinal-drift risk (AGREED A1+B2)user/v1/user_profile.proto:78
Line 78 defines
enum SubscriptionTierwith ordinals 0–3 (UNSPECIFIED/FREE/PRO/FAMILY). Verified:realtime/v1/connect.proto:94–104carries an identical enum. Wire-identical at HEAD, but there is no protobuf import,buf breakingrule, or CI gate enforcing ordinal parity across the two packages.Failure mode: A future PR adds
SUBSCRIPTION_TIER_ENTERPRISE = 4touser.v1.SubscriptionTierwithout updatingrealtime.v1.SubscriptionTier. Both files compile. The identity-service mapper receives ordinal 4, the realtime-side deserialiser silently coerces it toSUBSCRIPTION_TIER_UNSPECIFIED = 0, and affected users are silently downgraded at channel-subscribe time.Recommended fix (this PR or immediate follow-on): Promote the canonical definition to
shared/v1/subscription_tier.protoand import it in bothuser/v1/user_profile.protoandrealtime/v1/connect.proto. Alternatively, add a CI diff-script that validates ordinal parity between the two files on every push. File a tracked issue so the concern survives merge.[MINOR]
ParentalControlStateduplicated fromrealtime.v1— silent drift could silently disable parental locks (AGREED A2+B2)family/v1/family_context.proto:97
Line 97 defines
message ParentalControlStatewith 4 bool fields (ordinals 1–4). Verified:realtime/v1/connect.proto:107–116carries an identical message with the same field names and numbers. No import or CI gate enforces parity.Failure mode (higher-consequence than the tier drift): A fifth lock flag (e.g.
feature_lock_media = 5) is added tofamily.v1.ParentalControlStatebut missed inrealtime.v1.ParentalControlState. The new field is silently dropped by the Centrifugo Validate deserialiser, yieldingfalsefor the new lock across all channel-subscribe checks — a parent believes a lock is active but it is not enforced. This is a safety regression, not merely a feature-gate miss.Recommended fix: Promote
ParentalControlStatetoshared/v1/parental_control.protoand import it in bothfamily/v1/family_context.protoandrealtime/v1/connect.proto. Alternatively, add a CI ordinal-parity check. File a tracked issue so the concern survives merge.[MINOR]
has_familybool is a redundant discriminant alongside the message-typedfamily_idfield — inconsistent-state nil-dereference riskfamily/v1/family_context.proto:72
Line 72:
bool has_family = 1;. Line 77:shared.v1.FamilyId family_id = 2;.FamilyIdis a proto3 message type (confirmed inshared/v1/ids.proto:22), so it already carries field presence — it is nil/unset when absent. Having bothhas_familyandfamily_idintroduces two independent encodings of "user has no family" that can silently disagree.Failure mode: A server-side bug sets
has_family = truebut omitsfamily_id. Any caller that checkshas_familybefore dereferencingfamily_idwill hit a nil pointer. The same structural ambiguity applies toparental_state(line 86): a nilParentalControlStateand a zero-value one with all flagsfalseare indistinguishable without thehas_familygate, buthas_family = falsewith a non-nilparental_stateis also a valid inconsistency.Recommended fix: Use a
oneofto make the discriminant structural:Alternatively, drop
has_familyentirely, document thatfamily_id == nil↔ no family, and clarify that a nilparental_statealso means no family (not a zero-value snapshot).[INFO]
SUBSCRIPTION_TIER_UNSPECIFIED = 0comment should mandate fail-closed behaviour at the calleruser/v1/user_profile.proto:81
Line 81:
SUBSCRIPTION_TIER_UNSPECIFIED = 0;with comment "never published; indicates missing entitlement (caller-side bug or DB anomaly)." This describes what the value means but does not specify the required caller response.Risk: Proto3 omits fields whose value equals the zero/default on the wire. A partial-serialisation fault, network truncation, or deserialisation error producing a missing
subscription_tierfield is indistinguishable from an explicit DB anomaly signal — both yieldSUBSCRIPTION_TIER_UNSPECIFIEDat the caller. Without a documented contract, an identity-service implementation might silently mapUNSPECIFIED → FREE, granting free-tier access on any serialisation fault.Recommended fix — add one sentence to the comment:
This aligns with the fail-closed posture already established for
NOT_FOUNDsemantics on unknownuser_idin the RPC-level comment above.Verdict
CONDITIONAL_APPROVE
hib-pr-reviewer • round 1 • 4 findings (3m/1i) • 2026-05-24T16:44:23.236Z → 2026-05-24T16:47:26.456Z • posted-as: pr-reviewer-bot
R1 verdict findings (kept=4: 2 MINOR-agreed + 1 MINOR-unique + 1 INFO): (1+2) MINOR (agreed A+B) — SubscriptionTier + ParentalControlState duplicated in family.v1/user.v1 vs realtime.v1. No buf breaking rule or CI gate enforced ordinal parity. Failure modes: - SubscriptionTier drift: future SUBSCRIPTION_TIER_ENTERPRISE added to user.v1 but not realtime.v1 silently coerces to UNSPECIFIED across the wire → users downgraded at channel-subscribe. - ParentalControlState drift: a fifth lock flag added to family.v1 but missed in realtime.v1 yields false at the Centrifugo deserialiser → parent believes a lock is active but it is NOT enforced. Safety regression. Fix: removed both duplicate definitions; imported realtime/v1/connect.proto + reference realtime.v1.SubscriptionTier and realtime.v1.ParentalControlState directly. Single wire definition, drift class closed. realtime/v1 owns the wire type today because identity-service's existing PR-OPAQUE-3 codegen path already uses it; extracting to a future shared/v1 package is deferred to a separate cleanup PR. (3) MINOR (unique-to-one verified) — has_family bool was redundant with proto3 message-presence on family_id. Two independent encodings of "no family" risked inconsistent-state nil-deref: has_family=true + family_id=unset → nil dereference on the caller side. Fix: dropped has_family entirely. Caller uses field-presence (Java: hasFamilyId(); TS: family_id !== undefined; Python: HasField("family_id")) as the single source of truth. Added a presence-semantics table to the response docstring covering all 3 states (has-family + locks loaded; has-family + no locks active; no family). (4) INFO (unique-to-one) — SUBSCRIPTION_TIER_UNSPECIFIED zero-value comment described what it meant but not the required caller response. A partial-serialisation fault yielding UNSPECIFIED was indistinguishable from an explicit DB anomaly; without a contract, a naive identity-service mapper might coerce UNSPECIFIED → FREE → grant free-tier access on any wire fault. Fix: added a FAIL-CLOSED contract paragraph to the service- level docstring. Callers MUST treat UNSPECIFIED as "service unavailable / context unreliable" and refuse to mint (HTTP 503 + Retry-After). NEVER coerce → FREE silently. Same fail-closed treatment documented for unset parental_state in family/v1/family_context.proto. Verification: - buf lint → clean - buf build → clean - realtime/v1 imports resolve in both files - No new types introduced (deletion of duplicates closes the drift class without adding surface area)Show previous round
hib-pr-reviewer review — PR #1 (affinity-intelligence-rework/im2be-protobuf)
Round 2 — head
0bb271a9c74c, basemain, triggersynchronizeTL;DR: CONDITIONAL_APPROVE — kept 3 unique-but-verified findings (A:2 + B:1, all minor), 0 dropped; no blocking issues.
Summary
Round 2 Arbitration — all unique findings verified, all kept
Methodology
Verification details
A-1 (family_context.proto:51) — KEPT. Lines 50-52 confirmed:
"set (live snapshot from the \parental_control` row joined by family_id)". The RPC Semantics block's bullet 2 saysparental_state` will always be set for users WITH a family, but uses the word "joined" without specifying LEFT JOIN. The response-level presence table (lines 79-85) covers the no-locks case, but that table is on the message docstring, not the RPC semantics block — which is the first location an implementor reads. The gap is real and the safety consequence (INNER JOIN → unset parental_state → fail-closed disconnect for all lock-free families) is correctly characterised.A-2 (user_profile.proto:87 + family_context.proto:105) — KEPT. Both verbatim occurrences confirmed:
user/v1/user_profile.protolines 87-88:"and is deferred to a separate cleanup PR. Reviewer R1 MINOR finding (duplication risk closed)."family/v1/family_context.protolines 105-106:"PR. Reviewer R1 MINOR finding (duplication risk closed)."Review-tracking metadata that will be emitted verbatim by protoc-gen-doc / buf generate.
B-1 (family_context.proto:87) — KEPT. Lines 87-88 confirmed:
"Callers MUST distinguish \"unset parental_state\" (server error, fail-closed)"— unqualified. Presence table lines 84-85 explicitly lists"no family (fresh account) → family_id unset, parental_state unset"as a valid state. Field-level comment at line 108 correctly qualifies"(with family_id set)", but the message docstring is what code-generator consumers and SDK tooling read first. Contradiction confirmed; severity unchanged at minor.Outcome
Kept 3 findings (all unique-to-one, all verified). 0 agreed findings. 0 dropped findings. All minor → CONDITIONAL_APPROVE.
Blast Radius
The diff introduces two new exported gRPC service contracts (FamilyContextService.GetFamilyForUser and UserProfileService.GetUserProfile) consumed by identity-service at Centrifugo token mint time. Any caller implementing these RPCs based on the ambiguous docstrings will affect all realtime connections for lock-free family members and fresh sign-ups respectively. The contracts cross service boundaries (identity-service → family-service + user-service) and the fail-closed semantics make documentation precision safety-critical.
BLAST_SCORE: 6/10
Risk Indicators
FamilyContextService.GetFamilyForUser,UserProfileService.GetUserProfile,realtime.v1.ParentalControlState,realtime.v1.SubscriptionTierCI status (head
0bb271a9c74c)No CI checks reported for this commit.
Related PRs
Findings (3)
[MINOR] RPC docstring silent on "family exists, no parental_control row" join semantics — INNER JOIN implementation would disconnect all lock-free families
family/v1/family_context.proto:51
Lines 50-52:
The phrase "joined by family_id" does not specify join type. The response-level presence table (lines 79-85) correctly covers the "has family, no locks" case (
parental_stateset with all flags false), but it lives on the response message docstring — not the RPC Semantics block, which is where an implementor first looks when writing the server.An implementation using an INNER JOIN between
family_membersandparental_controlonfamily_idreturnsparental_stateunset for any family that has never opened parental-control settings. The caller (identity-service) then hits the fail-closed branch (lines 60-63) and disconnects the realtime client with a 4403. Every user in a lock-free family becomes unable to connect.Required fix: add a bullet to the Semantics block (after the "WITH a family" bullet, before NOT_FOUND), e.g.:
This directly mirrors the response-level table and removes the implementor ambiguity.
[MINOR] Review-process artifact "Reviewer R1 MINOR finding (duplication risk closed)." committed to field comments — will appear in generated SDK docs
user/v1/user_profile.proto:87
Two confirmed locations:
user/v1/user_profile.protolines 87-88:family/v1/family_context.protolines 105-106:This is internal review-process metadata with no meaning to external SDK consumers. It will be emitted verbatim by
protoc-gen-doc, the Java Javadoc plugin, and any TypeScript/Go comments produced bybuf generate.Required fix: strip
"Reviewer R1 MINOR finding (duplication risk closed)."from both field comments. The immediately preceding sentence ("extracting to a third 'shared/v1' package … is deferred to a separate cleanup PR") already captures the technical rationale that belongs in the contract.[MINOR] Message docstring labels ALL unset
parental_stateas "server error" — contradicts the presence table for the valid no-family casefamily/v1/family_context.proto:87
Lines 87-88:
This is unqualified. The presence table immediately above (lines 84-85):
lists
parental_stateunset as a valid, non-error response when the user has no family.The field-level comment at line 108 correctly qualifies:
"Unset when family_id is unset. Callers MUST treat unset (with family_id set) as fail-closed"— but the message docstring at line 87 is what code-generator consumers and SDK tooling surface first. An implementor reading only the message docstring will treat the no-family case (fresh sign-up) as a fail-closed disconnect, silently breaking every account that has never joined a family.Required fix (doc-only, no wire change):
Verdict
CONDITIONAL_APPROVE
hib-pr-reviewer • round 2 • 3 findings (3m) • 2026-05-24T16:55:00.362Z → 2026-05-24T16:57:42.945Z • posted-as: pr-reviewer-bot
R2 verdict findings (kept=3 all MINOR; 0 dropped): (1) MINOR family/v1/family_context.proto:51 — RPC docstring silent on "family exists, no parental_control row" join semantics. An INNER JOIN implementation would silently drop lock-free families from the response, then the caller's fail-closed contract would disconnect every such family at Centrifugo connect time (fail-OPEN-style regression dressed as fail-closed). Fix: docstring now explicitly mandates LEFT-JOIN semantics: family with no parental_control row → parental_state SET with all flag fields false (zero-lock snapshot), NOT unset. Added rationale describing what the INNER-JOIN failure mode would look like to a future maintainer. (2) MINOR user/v1/user_profile.proto:87 + family_context.proto:106 — review-process artifact strings "Reviewer R1 MINOR finding (duplication risk closed)." were committed in field comments and would have appeared in generated SDK docs (Java/TS/Python codegen propagates field comments to public JavaDoc / TSDoc / docstrings). Fix: removed both artifact strings; the comment text now describes only the wire semantics, no review provenance. (3) MINOR family/v1/family_context.proto:87 — message docstring labelled ALL unset parental_state as "server error", which contradicted the service-level docstring's row 3 (no family = family_id unset + parental_state unset, LEGITIMATE state). Two encodings of "unset" couldn't be distinguished by the callers reading the message docstring alone. Fix: replaced narrative paragraphs with a 4-row state table (family_id × parental_state combinations → meaning). Row 3 (both unset = no family) is explicitly distinguished from row 4 (family_id set + parental_state unset = INVALID, fail-closed). Callers MUST NOT treat row 3 as error. Verification: - buf lint → clean - buf build → clean - No "Reviewer R1" string remains (grep clean)Show previous round
hib-pr-reviewer review — PR #1 (affinity-intelligence-rework/im2be-protobuf)
Round 3 — head
b85b5462bcf1, basemain, triggersynchronizeTL;DR: NEEDS_WORK — kept 2 unique-to-A (both verified) + 1 unique-to-B (verified); 1 major blocking finding on is_admin revocation staleness, 2 minor findings on incomplete presence table and circular-import tech-debt.
Summary
Round 3 Arbitration — affinity-intelligence-rework/im2be-protobuf#1
Memora state: No prior run history found (no memory returned for either tags_any:[pr-reviewer,im2be-protobuf] or tags_all scoped to this PR); treating as first arbitration despite round=3. Memora create failed due to system-side tag allowlist constraint (auto-suggested tag 'memora/issues' rejected); persistence skipped — not a reconciliation error.
File reads: Both
family/v1/family_context.proto(132 lines) anduser/v1/user_profile.proto(94 lines) read in full.Finding A1 (minor, unique-to-A) — VERIFIED and KEPT: The 2×2 truth table at lines 94–110 of
family_context.protodocuments rows 1–4 but the{family_id=unset, parental_state=set}combination is genuinely absent. Line 108 asserts "row 4 is the ONLY combination that triggers fail-closed" — this is factually wrong if the server can emitparental_statewithoutfamily_id; callers have no contract for that state and would silently fall into the row-3 (no-family / benign) branch.Finding A2 (major, unique-to-A) — VERIFIED and KEPT: Lines 52–56 of
user_profile.protoexplicitly document thatsubscription_tierstaleness is recoverable (subscribe_proxy re-checks at channel-subscribe time). Lines 91–93 defineis_adminwith zero staleness or revocation guidance. The 300s Redis TTL (family_context.proto line 17) means a revoked admin retains elevated access for up to 5 minutes with no documented remediation. This is a real security-contract gap on a high-sensitivity field.Finding B1 (minor, unique-to-B) — VERIFIED and KEPT:
family/v1/family_context.protoline 31 anduser/v1/user_profile.protoline 29 both importrealtime/v1/connect.proto. Lines 124 and 85 respectively acknowledge the deferredshared/v1extraction. The circular-import risk is prospective but real asrealtime/v1grows. Kept as tech-debt signal.Disposition: 2 unique-to-A verified and kept; 1 unique-to-B verified and kept; no agreed findings (no overlap between A and B); 0 dropped.
Blast Radius
The diff introduces two new gRPC contract files that define the mint-side context RPCs consumed by identity-service at token-mint time. Both contracts feed into the Redis-cached Centrifugo opaque ticket shared across family-service, user-service, and identity-service. The
is_adminfield on the exportedGetUserProfileResponsetouches the auth/privilege boundary used by admin-panel access, impersonation, and audit-log read — an exported surface with meaningful blast radius if the staleness contract is misimplemented by the Redis flush path.BLAST_SCORE: 6/10
Risk Indicators
GetFamilyForUser,GetUserProfile,is_admin,ParentalControlState,parental_stateCI status (head
b85b5462bcf1)No CI checks reported for this commit.
Findings (3)
[MINOR] Presence table omits
{family_id unset, parental_state set}— callers have no contract for this server-bug statefamily/v1/family_context.proto:103
The 4-row truth table (lines 94–110) covers only 3 of the 4 possible two-field-presence combinations. The combination
{family_id=unset, parental_state=set}is missing. Line 108 states: "row 4 is the ONLY combination that triggers fail-closed" — this is incorrect: a buggy server can emitparental_statewithoutfamily_id, and callers have no documented instruction for that state. They will silently fall through to the row-3 (no-family) branch, discarding the parental-control snapshot and proceeding fail-OPEN.Required fix — add after row 4 (line 105):
And update line 108: change
"row 4 is the ONLY combination"→"rows 4 and 5 are the ONLY combinations".[MAJOR]
is_adminfield has no staleness/revocation guidance — undocumented 300 s privilege window on admin revocationuser/v1/user_profile.proto:91
The service docstring (lines 52–56) explicitly states that stale
subscription_tieris recoverable because Centrifugo subscribe_proxy re-checks tier at channel-subscribe time. No equivalent statement exists foris_admin. The field governsadmin-panel access, impersonation rights, audit-log read(lines 91–92) — all highly sensitive capabilities.When an admin is revoked, the minted ticket cached in Redis (
SET EX 300, family_context.proto line 17) continues to carryis_admin=truefor up to 300 seconds. There is no documented remediation path in this contract.The field comment MUST state one of:
is_adminat channel-subscribe time (safe — revocation takes effect before any channel is joined), ORSuggested field comment at line 91:
[MINOR]
realtime/v1reverse-import coupling — circular-dependency risk accumulates with each newrealtime/v1protofamily/v1/family_context.proto:124
Both
family/v1/family_context.proto(line 31) anduser/v1/user_profile.proto(line 29) importrealtime/v1/connect.prototo reuseParentalControlStateandSubscriptionTier. Lines 124 and 85 respectively acknowledge "extracting to a third shared/v1 package is deferred."The import direction is problematic:
user/v1andfamily/v1are mint-side producer contracts;realtime/v1is the Centrifugo consumer-side contract. Any futurerealtime/v1proto that needs to reference auser/v1orfamily/v1type will cause protoc to reject the build with a circular-import error.realtime/v1already has five proto files (connect,rpc,subscribe,envelope,publish) and is growing; the probability of an accidental cross-reference grows with each addition.The import-based reuse is the correct immediate fix. The deferred action is to schedule the
shared/v1extraction — movingSubscriptionTierandParentalControlStatetoshared/v1/tiers.protoandshared/v1/parental_control.proto— beforerealtime/v1grows further and makes the refactor more expensive.Verdict
NEEDS_WORK
hib-pr-reviewer • round 3 • 3 findings (1M/2m) • 2026-05-24T17:04:41.027Z → 2026-05-24T17:07:07.367Z • posted-as: pr-reviewer-bot
R3 verdict findings (kept=3; all addressed; no NEEDS_WORK blockers): (1) MAJOR user/v1/user_profile.proto:91 — `is_admin` field had no staleness/revocation guidance. The Centrifugo opaque ticket SET EX 300 (per ADR-0002 §3) creates a 5-minute privilege window after admin revocation during which the cached ticket continues to assert `is_admin=true`. The field gates Centrifugo channel admin access (audit streams, admin-only channels); no remediation path was documented. Fix: added a STALENESS/REVOCATION CONTRACT block to the field docstring mandating that Centrifugo subscribe_proxy MUST re-check is_admin at channel-subscribe time for any admin- gated channel (via UserProfileService.GetUserProfile or a lighter IsAdmin RPC if subscribe-latency budgets demand). This becomes a contract for the PR-OPAQUE-5 / PR-OPAQUE-6 implementer. Also clarified the boundary: the contract concerns Centrifugo channel subscription only; admin-panel uses its own session-based auth via admin-service and is unaffected. (2) MINOR family/v1/family_context.proto:103 — presence table omitted row 5 `{family_id unset, parental_state set}`. A buggy server emitting parental_state without family_id would fall through to the row-3 (no-family) branch and silently drop the parental_control snapshot — fail-OPEN regression masked by the missing row. Fix: added row 5 to the truth table with the same "INVALID — server bug; caller MUST fail-closed" semantics as row 4. Updated the trailing note: rows 4 AND 5 trigger fail-closed (different shapes, same caller contract). (3) MINOR family/v1/family_context.proto:124 + user/v1/user_profile.proto:85 — circular-import risk: family/v1 + user/v1 importing realtime/v1 inverts the natural producer→consumer flow. Any future realtime/v1 proto wanting to reference family/v1 or user/v1 would hit a circular import that protoc rejects. Fix: added SCHEDULED FOLLOW-UP block in both files documenting the eventual shared/v1 extraction (parental_control.proto + tiers.proto). Tracks in meta-repo TODO.md as PR-OPAQUE-4-FOLLOWUP-shared-proto-extraction. The import-based reuse stays as the immediate fix; the extraction lands before realtime/v1 grows further. Verification: - buf lint → clean - buf build → clean - All 7 PR-OPAQUE-4a R1+R2+R3 findings now closed (4 R1 + 3 R2 + 3 R3 = 10 findings handled across 4 rounds)Show previous round
hib-pr-reviewer review — PR #1 (affinity-intelligence-rework/im2be-protobuf)
Round 4 — head
a4f27e5d86d1, basemain, triggersynchronizeTL;DR: CONDITIONAL_APPROVE — kept 2 agreed+verified minor findings (reviewer-attribution labels in proto field comments will leak into generated SDK docs); no blocking issues; all 9 prior R1–R3 findings confirmed resolved.
Summary
Round 4 Arbitration — Reconciliation
Memora recall: No prior run history found (tags allowlist prevented persistence in all attempts — system config issue, not arbitration-blocking). Treated as first stored run.
Agreed findings (Rule 1 — gold): Both A and B independently flagged the exact same two findings with identical file+line citations and materially equivalent bodies. No divergence.
Verification (Rule 2/3):
family/v1/family_context.proto:134— Read confirmed: line 134 reads// SCHEDULED FOLLOW-UP (Reviewer R3 MINOR — circular-import. Attribution label present. ✅ KEPT.user/v1/user_profile.proto:98— Read confirmed: line 98 reads// STALENESS / REVOCATION CONTRACT (Reviewer R3 MAJOR):. Attribution label present. ✅ KEPT.Prior rounds: All R1–R3 findings confirmed resolved by both reviewers (9 closures). No re-raised prior findings.
Recurring pattern note: Reviewer-attribution parentheticals (
Reviewer RN SEVERITY) are being introduced into.protofield comments during fix commits — the same class fixed in R2. The team should add a pre-commit or CI lint rule (e.g.grep -rn 'Reviewer R[0-9]' **/*.proto) to catch this automatically before future rounds require it.Kept 2 agreed+verified minor findings. Dropped 0. No blocking or major issues.
Blast Radius
Two new proto files introducing gRPC service contracts consumed by identity-service at mint time. The exported surface is narrow (one RPC per file) but auth-adjacent: GetUserProfile carries the is_admin flag and GetFamilyForUser carries parental-control state, both of which feed Centrifugo channel access decisions. The reviewer-attribution findings are cosmetic and carry no runtime blast radius, but the proto contracts themselves are high-sensitivity given their role in the mint flow.
BLAST_SCORE: 4/10
Risk Indicators
GetUserProfile,GetFamilyForUser,FamilyContextService,UserProfileServiceCI status (head
a4f27e5d86d1)No CI checks reported for this commit.
Findings (2)
[MINOR] Review-process artifact
(Reviewer R3 MINOR — circular-import tech-debt)will appear verbatim in generated SDK docsfamily/v1/family_context.proto:134
Line 134 reads:
The parenthetical
(Reviewer R3 MINOR — circular-import tech-debt)is an internal review-severity label with no meaning to downstream SDK consumers.protoc --doc_out,buf generate, Javadoc, godoc, TypeScript/** */blocks, and grpc-gateway Swagger all reproduce field comments verbatim. This is the same class asReviewer R1 MINOR finding (duplication risk closed)removed fromuser_profile.proto:87in R2.Required fix — replace line 134 with:
The substantive explanation in lines 135–146 (import-direction inversion, circular-import risk, extraction follow-up, TODO.md tracking) is well-formed public API documentation and must be kept exactly as-is.
[MINOR] Review-process artifact
(Reviewer R3 MAJOR)will appear verbatim in generated SDK docs foris_adminuser/v1/user_profile.proto:98
Line 98 reads:
The parenthetical
(Reviewer R3 MAJOR)is an internal review-severity label identical in class to the R2 finding atuser_profile.proto:87. It has no meaning to SDK consumers but will appear in generated Javadoc, godoc, and IDE hover docs for theis_adminfield.Required fix — replace line 98 with:
The substantive 300 s privilege-window and mitigation contract in lines 99–117 (Redis TTL, revocation window, subscribe_proxy re-check mandate, admin-panel carve-out) is well-formed public API documentation and must be kept exactly as-is.
Verdict
CONDITIONAL_APPROVE
hib-pr-reviewer • round 4 • 2 findings (2m) • 2026-05-24T17:11:49.152Z → 2026-05-24T17:13:56.439Z • posted-as: pr-reviewer-bot
R4 verdict findings (kept=2 MINOR; both same class as the R2 review- artifact-leak finding I introduced new violations of in R4): (1) MINOR family/v1/family_context.proto:134 — 'SCHEDULED FOLLOW-UP (Reviewer R3 MINOR — circular-import tech-debt)' in field comment would leak into generated SDK docs. (2) MINOR user/v1/user_profile.proto:98 — 'STALENESS / REVOCATION CONTRACT (Reviewer R3 MAJOR)' in is_admin field comment same class. Both fixed by removing the parenthetical reviewer-attribution labels. The substantive content (SCHEDULED FOLLOW-UP block + STALENESS/REVOCATION CONTRACT block) stays — only the review provenance is removed. This is the third occurrence of the review-artifact-leak class (R2 caught the first two, R4 catches the two I introduced in R3 + R4). Lesson logged: AVOID 'Reviewer R<N>' attribution strings in committed proto/code comments; they leak into generated docs. The PR description / commit message is the right home for review provenance. Verification: - buf lint + build → clean - grep -n 'Reviewer R' family/v1/* user/v1/* → 0 matches (audit complete)Show previous round
hib-pr-reviewer review — PR #1 (affinity-intelligence-rework/im2be-protobuf)
Round 5 — head
8f87fc0f8216, basemain, triggersynchronizeTL;DR: CONDITIONAL_APPROVE — 4 minor findings kept this round.
Summary
Arbiter reconciled 2 (A) + 2 (B) → 4 findings.
CI status (head
8f87fc0f8216)No CI checks reported for this commit.
Findings (4)
[MINOR] RPC docstring fail-closed clause omits Row 5 — callers reading only the method comment miss the family_id-unset + parental_state-set server-bug case
family/v1/family_context.proto:71
Lines 71–78 enumerate two combinations:
This covers Row 4 (SET + UNSET → fail-closed) and Row 3 (UNSET + UNSET → normal). Row 5 — (family_id UNSET, parental_state SET) → INVALID server bug, caller MUST fail-closed — is absent from the RPC method docstring. It appears only in the response message docstring at lines 106–114, which proto tooling (grpc-gateway stub generators, Buf docs, IDE hover) does not surface at the call site. A caller who reads only the RPC docstring will believe the only fail-closed trigger is "unset parental_state + set family_id" and may silently treat a Row 5 response as a valid no-family state, granting channel access without family context.
Fix — append after line 78, before the
rpcdeclaration:[MINOR] Semantics section silent on INVALID_ARGUMENT for nil/absent user_id — implementers may conflate malformed request with NOT_FOUND
family/v1/family_context.proto:64
Line 64 (and the parallel line 50 in
user/v1/user_profile.proto):The
user_idfield isshared.v1.UserId, a proto3 message type with field presence. If the caller omits the field entirely (a mint-path bug), the server receives a nil/zero-value struct. "Not found in the users table" is semantically distinct from "field structurally absent from the request". An implementer who reads only this bullet may pass a nil UUID to the database and let it produce a NOT_FOUND, causing callers to misclassify a malformed-request error as a user-lookup miss and potentially retry a fundamentally broken request.Fix — add a bullet to both services' Semantics sections:
Applies to both
GetFamilyForUser(family_context.proto:64) andGetUserProfile(user_profile.proto:50).[MINOR] Presence table rows 3–5 drop column-separator pipes, breaking ASCII-table alignment in generated docs
family/v1/family_context.proto:102
Rows 1 and 2 continuation lines (98–100) correctly use the pipe-separator pattern:
Rows 3–5 continuation lines (102, 104–105, 107–114) instead use bare indentation with no pipe characters:
This breaks column alignment in both direct proto reading and in
protoc-gen-doc-generated HTML/Markdown. The presence table is load-bearing caller-contract documentation; a misread row 4 or 5 could lead a caller to treat an INVALID combination as the no-family case. Fix by using the same| | |prefix on every continuation line for rows 3–5.[MINOR]
subscription_tierfield comment contains internal process cross-reference that leaks verbatim into generated SDK docsuser/v1/user_profile.proto:86
Lines 86–89:
The identifier
PR-OPAQUE-4-FOLLOWUP-shared-proto-extractionand themeta-repo TODO.mdreference are pure internal process bookkeeping with no semantic content for Java/Kotlin/Go SDK consumers. The full architectural explanation (import-direction risk + extraction plan) is already present atfamily/v1/family_context.proto:134–145; a consumer ofsubscription_tiergains nothing from a 4-line internal tracker reference in the field comment.Fix — remove lines 86–89 entirely. If a breadcrumb is desired, replace with a single line without the internal identifier:
Verdict
CONDITIONAL_APPROVE
hib-pr-reviewer • round 5 • 4 findings (4m) • 2026-05-24T17:22:17.786Z → 2026-05-24T17:25:21.621Z • posted-as: pr-reviewer-bot • [bookkeeping fallback]
R5 verdict findings (kept=4, all MINOR; all addressed): (1) family_context.proto:71 — RPC docstring fail-closed clause only mentioned row 4; row 5 visible only in the response message docstring (not surfaced at the call site by proto tooling). Caller reading only the RPC comment might treat a row-5 response as no-family. Fix: replaced the narrative clause with a 3-bullet enumeration covering rows 3+4+5 explicitly + cross-reference to the 5-row presence table. (2) family_context.proto:64 + user_profile.proto:50 — Semantics bullet on "Unknown user_id → NOT_FOUND" was silent on the NIL/STRUCTURALLY-ABSENT user_id case. Implementer might pass nil through to the DB and let it produce NOT_FOUND, causing callers to misclassify malformed-request as user-lookup miss + retry indefinitely. Fix: added explicit "INVALID_ARGUMENT" bullet to both Semantics sections distinguishing structural absence from missing-row. (3) family_context.proto:102 — presence table rows 3-5 continuation lines dropped column-separator pipes, breaking ASCII-table alignment in protoc-gen-doc output. Fix: added consistent `| | |` prefix to all continuation lines in rows 3+4+5 (matches the existing format of rows 1+2). (4) user_profile.proto:86 — `subscription_tier` field comment contained the internal-process identifier `PR-OPAQUE-4- FOLLOWUP-shared-proto-extraction` and `meta-repo TODO.md` cross-reference that would leak verbatim into generated SDK docs (Java/TS/Python). The full architectural explanation is already at family_context.proto:134-145; subscription_tier consumers gain nothing from a 4-line internal tracker. Fix: replaced with single-line breadcrumb pointing readers to the family_context.proto comment instead. Verification: - buf lint + build → clean - All 16 PR-OPAQUE-4a findings closed across R1+R2+R3+R4+R5 (4+3+3+2+4) rounds.Show previous round
hib-pr-reviewer review — PR #1 (affinity-intelligence-rework/im2be-protobuf)
Round 6 — head
e7c3b48e6d31, basemain, triggersynchronizeTL;DR: CONDITIONAL_APPROVE — kept 2 minor findings: 1 agreed (A+B) on family_context.proto SCHEDULED FOLLOW-UP block leaking internal PM metadata into generated SDK docs; 1 unique-to-B verified on user_profile.proto dangling cross-file navigation pointer; no blocking issues.
Summary
Round 6 Arbitration
Memora recall: No prior run history found in Memora (first-time store for this PR despite round 6); created new entry ID 221.
Verification performed:
family/v1/family_context.protolines 135–159 → confirmed lines 141–152 contain the SCHEDULED FOLLOW-UP block with internal PM tracking tokens (PR-OPAQUE-4 post-merge follow-upat line 146–147,meta-repo TODO.md as PR-OPAQUE-4-FOLLOWUP-shared-proto-extractionat lines 151–152,identity-service's vendored proto copy + Java codegen pathat line 150). Finding A (line 152) and B (line 141) describe the same block — AGREED, kept with canonical start-of-block citation at line 141.user/v1/user_profile.protolines 83–94 → confirmed lines 89–90 read// Import-direction follow-up: see family/v1/family_context.proto/// parental_state comment.— a pure cross-file navigation pointer with no self-contained meaning in generated SDK docs. UNIQUE-TO-B finding verified, kept.Disposition: Kept 2 findings (1 agreed A+B, 1 unique-to-B verified). Nothing dropped.
Blast Radius
Both files are new additions (net +280 lines, zero deletions) defining the public proto contract for family-service and user-service mint-side RPCs consumed by identity-service at Centrifugo token mint time. Impact propagates to protoc-generated stubs in Java, Go, Python, and TypeScript for any service importing these protos. Scope is contained to two new RPC surfaces not yet in production — no existing callers are broken by this diff.
BLAST_SCORE: 4/10
Risk Indicators
FamilyContextService.GetFamilyForUser,UserProfileService.GetUserProfileCI status (head
e7c3b48e6d31)No CI checks reported for this commit.
Findings (2)
[MINOR] SCHEDULED FOLLOW-UP block (lines 141–152) leaks internal PM tracking metadata verbatim into generated SDK docs for
parental_statefieldfamily/v1/family_context.proto:141
Lines 141–152 of the
parental_statefield comment embed three categories of internal metadata that protoc emits verbatim into generated SDK documentation (Java Javadoc, Python docstrings, Go godoc, TypeScript JSDoc):PR-OPAQUE-4 post-merge follow-up(lines 146–147)Tracked in meta-repo TODO.md as PR-OPAQUE-4-FOLLOWUP-shared-proto-extraction(lines 151–152)identity-service's vendored proto copy + Java codegen path(line 150)External SDK consumers see sprint-tracking tokens and internal toolchain paths that are unintelligible outside the team. The architectural concern itself (inverted import direction, proposed extraction to
shared/v1) is valid and should be preserved.Concrete fix — replace lines 141–152 with:
The detailed migration plan (vendored copy, Java codegen path, lockstep update) belongs in the linked issue or PR description, not committed proto source.
[MINOR] Cross-file navigation pointer in
subscription_tiercomment is a dangling reference in generated SDK docsuser/v1/user_profile.proto:89
Lines 89–90 read:
This is a source-tree navigation instruction assuming the reader has
family/v1/family_context.protoopen. In generated SDK documentation (e.g. the Java class forUserProfileResponse) this renders as a dangling reference that is meaningless to SDK consumers — they have no proto source tree, only the generated class or module.The import-direction concern is already covered by the file-level header (lines 16–23 of
user_profile.proto) and in full infamily/v1/family_context.protolines 141–152. This inline cross-reference adds no information for external consumers.Fix: replace lines 89–90 with a self-contained one-liner, e.g.:
or delete both lines — the file-level header already covers this.
Verdict
CONDITIONAL_APPROVE
hib-pr-reviewer • round 6 • 2 findings (2m) • 2026-05-24T17:31:14.754Z → 2026-05-24T17:32:56.053Z • posted-as: pr-reviewer-bot
R6 verdict findings (kept=2 MINOR; both same leak class): (1) MINOR family/v1/family_context.proto:141-152 — SCHEDULED FOLLOW-UP block (kept after R5 doc-polish) STILL leaks PM metadata: "PR-OPAQUE-4-FOLLOWUP-shared-proto-extraction" identifier + "meta-repo TODO.md" reference + the entire extraction-plan narrative would appear in every Java/TS/ Python codegen SDK doc for the parental_state field. (2) MINOR user/v1/user_profile.proto:86 — cross-file pointer to family_context.proto's parental_state comment was dangling after I removed the technical content there (R6 same fix deleted the target paragraph). Fix: REMOVED the SCHEDULED FOLLOW-UP block entirely from family_context.proto + the dangling cross-reference from user_profile.proto. The substantive technical content (shared/v1 extraction plan, import-direction risk) lives in: - meta-repo TODO.md (PR-OPAQUE-4-FOLLOWUP-shared-proto-extraction) - Memora memory #3448 (extraction milestone) - This PR's description on Forgejo Proto field comments now contain ONLY wire-semantic content; proto process-tracking does not bleed into SDK docs. Cumulative R-cycle: 18 findings closed across 6 rounds (R1=4, R2=3, R3=3, R4=2, R5=4, R6=2). All findings have been the same "docs polish" class for the last 4 rounds — converging now that all internal-process refs are excised. Verification: - grep -nE 'FOLLOW-UP|TODO\.md|meta-repo|PR-OPAQUE|Reviewer R' family/v1/* user/v1/* → 0 matches - buf lint + build → cleanShow previous round
hib-pr-reviewer review — PR #1 (affinity-intelligence-rework/im2be-protobuf)
Round 7 — head
b414ef361930, basemain, triggersynchronizeTL;DR: CONDITIONAL_APPROVE — kept 4 findings (2 agreed 'R1 review' artifacts, 1 unique-to-B 'Option A rejected 2026-05-24' artifact verified, 1 unique-to-A buf lint deferred/info verified); all minor or info, no blocking issues.
Summary
Arbitration — Round 7
Recalled Memora context: no prior memories found for this repo/PR despite being round 7 (Memora is unpopulated). Memora persistence attempted but blocked by tag-allowlist constraints and permission gates; falls through gracefully.
Verification performed
family/v1/family_context.proto:23— Read confirmed:// silent-drift class identified at R1 review (a duplicate definition thatfamily/v1/family_context.proto:31— Read confirmed:import "realtime/v1/connect.proto"user/v1/user_profile.proto:20— Read confirmed:// class identified at R1 review (a duplicate enum where a futureuser/v1/user_profile.proto:10-11— Read confirmed:// Why a dedicated RPC (and not extending the JWT claim shape — Option A/// rejected 2026-05-24): JWT TTL (15-60min) > many state changesReconciliation outcome
family_context.proto:23anduser_profile.proto:20for identical "R1 review" artifact — verified).user_profile.proto:10-11, "Option A rejected 2026-05-24" — verified at the cited lines; same class of generated-doc contamination as the agreed findings, grounded by Read).family_context.proto:31, no buf lint guard — deferred/info; import verified at line 31; finding is a legitimate architectural observation, not a speculative claim).Blast Radius
Two new proto files define gRPC service contracts (FamilyContextService, UserProfileService) consumed by identity-service at Centrifugo mint time. The contracts are exported surfaces used across at least three services (identity, family, user). All 4 kept findings are documentation-class defects in file-preamble comments — no wire format, field numbering, or service semantics are affected.
BLAST_SCORE: 5/10
Risk Indicators
GetFamilyForUser,GetUserProfileCI status (head
b414ef361930)No CI checks reported for this commit.
Findings (4)
[MINOR] "R1 review" process-reference in file-preamble leaks verbatim into generated SDK docs
family/v1/family_context.proto:23
Line 23 (Read-verified) contains:
File-level proto comments appearing before
syntax = "proto3"are included verbatim by protoc-gen-doc, the buf.build schema explorer, and as source-file header comments in generated Java/Go/Python stubs. The stringR1 reviewis an internal code-review round label carrying no meaning to downstream SDK consumers or future maintainers. The design rationale in the surrounding sentence is sound; only the six-word process attribution requires removal.Fix:
[MINOR] "R1 review" process-reference in file-preamble leaks verbatim into generated SDK docs
user/v1/user_profile.proto:20
Line 20 (Read-verified) contains:
Mirrors the
family_context.protoissue. File-level package comments receive the same protodoc treatment as field-level comments.R1 reviewis meaningless to any external consumer.Fix:
[MINOR] "Option A rejected 2026-05-24" bakes opaque option label + ephemeral date into generated SDK docs
user/v1/user_profile.proto:11
Lines 10–11 (Read-verified):
Option Ais an opaque internal design-option label (meaningful in the PR thread, meaningless in generated docs or in 18 months).2026-05-24is a process timestamp that will appear verbatim in SDK documentation. The architectural rationale — JWT TTL exceeds state-change intervals — is valuable and should be preserved without the process artifacts.Fix:
[INFO] No buf lint guard prevents future realtime/v1 circular-import accumulation
family/v1/family_context.proto:31
Line 31 (Read-verified):
import "realtime/v1/connect.proto". The file-preamble (lines 19–25) documents the architectural rationale for the import direction, but there is no mechanical guard to prevent a future proto author from importingfamily/v1back intorealtime/v1, closing the cycle. Proto circular imports are a hard compiler error for direct cycles but are easy to introduce indirectly via a newshared/v1message that transitively importsfamily/v1. As additional service protos accumulate imports ofrealtime/v1/connect.proto, the risk surface grows.This import is correct and well-documented — no change needed in this PR. Recommended follow-on: add a
buf lintrule orbuf.yamldepssection declaringrealtime/v1as a leaf package that MUST NOT import service packages, surfacing accidental cycles at CI time.Verdict
CONDITIONAL_APPROVE
hib-pr-reviewer • round 7 • 4 findings (3m/1i) • 2026-05-24T17:40:35.558Z → 2026-05-24T17:42:26.673Z • posted-as: pr-reviewer-bot
R7 verdict findings (kept=4: 2 agreed-MINOR file-preamble leaks + 1 unique-MINOR + 1 INFO buf-lint-deferred). The leak class has now been caught 5 times across rounds; tackling it comprehensively this round. Findings + fixes: (1+2) MINOR (agreed A+B) family_context.proto + user_profile.proto file preambles contained 'L0 T0 #6 PR-OPAQUE-4 (option b — gRPC fan-out)' + 'Stage B context (ADR-0002 §3a, drafted in PR-OPAQUE-4e)' + 'silent-drift class identified at R1 review' — all process bookkeeping that protoc emits to the generated SDK .java/.ts/.py file-level Javadoc. Fix: rewrote both file preambles to focus on wire semantics + architectural rationale only. Removed 'L0 T0 #6 PR-OPAQUE-4 (option b)', 'PR-OPAQUE-4e', 'Stage B context', 'R1 review', and 'Option A rejected 2026-05-24' (ephemeral-date artifact in user_profile.proto). The mint- flow + ownership + drift-prevention explanations stay because they are architectural content useful to downstream consumers. (3) MINOR (unique-to-B) user_profile.proto 'Option A rejected 2026-05-24' baked an internal option-label vocabulary + an ephemeral date into SDK docs. Fix: replaced 'Why a dedicated RPC (and not extending the JWT claim shape — Option A rejected 2026-05-24)' with 'Why a dedicated RPC (and not custom JWT claims)'. The substantive rationale (JWT TTL vs state-change frequency, fresh-truth requirement) stays. (4) INFO buf-lint guard suggestion — deferred per reviewer's 'info' tag. Tracked in meta-repo TODO.md as part of the PR-OPAQUE-4-FOLLOWUP-shared-proto-extraction work; adding a buf BREAKING rule for cross-package ordinal parity would require buf 1.x BREAKING rule authoring (non-trivial), deferred to the followup. Also caught one residual that wasn't formally in R7's findings but was the next predicted leak: user_profile.proto:104 is_admin MITIGATION CONTRACT block still mentioned 'for the PR-OPAQUE-5 / PR-OPAQUE-6 implementer' — fixed in same commit (removed the framing; technical content stays). Cumulative R-cycle: 22 findings closed across PR-OPAQUE-4a R1+R2+ R3+R4+R5+R6+R7 (4+3+3+2+4+2+4 = 22). The leak class is now EXHAUSTIVELY audited via grep: grep -nE 'PR-OPAQUE|R1 review|Option A rejected|2026-05| Reviewer R|TODO\.md|meta-repo|SCHEDULED' family/v1/* user/v1/* → 0 matches Verification: - buf lint + build → clean - Audit grep returns 0 matches (full leak-class closure)Show previous round
hib-pr-reviewer review — PR #1 (affinity-intelligence-rework/im2be-protobuf)
Round 8 — head
aeefc0afbafa, basemain, triggersynchronizeTL;DR: CONDITIONAL_APPROVE — 2 minor findings kept this round.
Summary
Arbiter reconciled 1 (A) + 1 (B) → 2 findings.
CI status (head
aeefc0afbafa)No CI checks reported for this commit.
Findings (2)
[MINOR]
parental_statefield comment omits row-5 fail-closed requirementfamily/v1/family_context.proto:144
The field-level comment at lines 140–144 covers rows 1+2 (set when family_id set), row 3 (unset when family_id unset), and row 4 (family_id set + parental_state unset → server bug, fail-closed), but is entirely silent on row 5: family_id unset + parental_state set (also a server bug requiring fail-closed).
The method docstring (lines 80–84) and the 5-row presence table (lines 112–120) do document row 5, but a developer reading only the per-field comment — the common path in IDE tooltips and generated SDK documentation — would never see the row-5 contract. They could observe
parental_statepopulated, skip thefamily_idpresence check, and proceed as if the user has a family, defeating the symmetric fail-closed requirement.Given that
parental_statecontrols child-lock enforcement, this is a safety-documentation gap.Proposed fix — append one sentence after the current line 144:
[MINOR] buf.yaml has no import-graph guard; both
family.v1anduser.v1now importrealtime/v1with no mechanical restrictionuser/v1/user_profile.proto:26
Line 26:
import "realtime/v1/connect.proto";buf.yaml(repo root, verified) declares a single flat module (buf.build/im2be/protobuf),deps: [],STANDARD+COMMENTSlint, andWIRE_JSONbreaking rules. There is nodisallow_imports, no per-package allow-list, and no multi-module split.buf's built-in acyclic import enforcement catches circular dependencies but does not prevent unauthorized unidirectional imports. A future committer can add
import "realtime/v1/connect.proto"in a third package (e.g.notification/v1/) and CI will pass silently. With two production packages (family.v1,user.v1) already coupling torealtime.v1, accumulation risk is materially higher than at round 7 (single-package coupling).Concrete fix options:
family/v1,user/v1, andrealtime/v1into separate buf module declarations (buf.yamlmodules:list) with explicitdepsentries — any newrealtime/v1consumer must add a module-level dep, making coupling visible in the PR diff.realtime/v1, and document approved importers inbuf.yamlcomments.The downstream failure mode: a future author imports a
realtime/v1message for convenience, thenrealtime/v1is refactored to import fromfamily/v1for a callback RPC — producing a circular dependency that fails at buf build with a cryptic cycle error, not at PR-review stage.Verdict
CONDITIONAL_APPROVE
hib-pr-reviewer • round 8 • 2 findings (2m) • 2026-05-24T17:49:47.479Z → 2026-05-24T17:52:14.690Z • posted-as: pr-reviewer-bot • [bookkeeping fallback]
R8 verdict findings (kept=2 MINOR): (1) family_context.proto:140-144 — `parental_state` field comment covered rows 1+2+3+4 of the presence table but was silent on row 5 (family_id unset + parental_state set = server bug). Developer reading only the per-field comment (the common path in IDE tooltips + generated SDK docs) could observe parental_state populated, skip the family_id presence check, and proceed as if the user has a family — defeating the symmetric fail-closed contract. Fix: expanded the per-field comment to explicitly cover BOTH row 4 and row 5; added a warning about "observing parental_state without verifying family_id" as the failure mode. (2) buf.yaml — no import-graph guard. Both family.v1 and user.v1 now import realtime/v1 with no mechanical restriction. The import direction is inverted (producer→consumer→realtime, when the natural direction would be the opposite). Risk accumulates as realtime/v1 grows. Fix: added a comment-enforced guard block to buf.yaml documenting the constraint + the proper long-term fix (shared/v1 extraction). A mechanical guard (multi-module split with explicit deps OR disallow_imports lint rule) requires the shared/v1 extraction to land first; both moves are bundled into the follow-up. Verification: - buf lint + build → clean 24 findings closed across 8 rounds (4+3+3+2+4+2+4+2 = 24).Show previous round
hib-pr-reviewer review — PR #1 (affinity-intelligence-rework/im2be-protobuf)
Round 9 — head
7b5ca38026a8, basemain, triggersynchronizeTL;DR: CONDITIONAL_APPROVE — kept 2 unique-to-one findings (both verified via Read at HEAD); no agreed findings this round; all 24 prior-round findings confirmed resolved.
Summary
Arbitration Round 9
Recalled Memora context: no prior run history found (tags blocked by permission constraints; attempted absorb also blocked). Treated as create-new path; Memora persistence attempted but failed due to permission constraints — findings reconciliation proceeds independently.
Verification
Finding A (unique-to-A) —
user/v1/user_profile.proto:12: Read at HEAD confirms line 12 reads exactly// Stage B architectural posture (clean service boundaries).This is an internal project-phasing label with no meaning to protoc-generated SDK consumers, matching the same artifact class cleaned from rounds 7–8. Lines 9–11 already state the complete rationale; the "Stage B" qualifier is purely internal jargon. KEPT.Finding B (unique-to-B) —
buf.yaml:21–24: Read at HEAD confirms lines 21–24 read "A mechanical guard ... is the proper long-term protection but requires the shared/v1 extraction to land first." Reviewer B's technical claim is correct: the multi-module split option (option 1) does not require movingParentalControlState/SubscriptionTierout ofrealtime/v1— declaring separate buf modules withrealtime/v1as an explicit dep forfamily/v1anduser/v1is sufficient to enforce the import graph mechanically. Only if the extraction itself (moving types intoshared/v1) is desired does the two steps need coordination. As written, the comment conflates the two options and may cause future contributors to delay the mechanical guard. KEPT.Both findings are minor. No prior-round findings are re-raised (all 24 confirmed resolved at HEAD by both reviewers).
Blast Radius
Diff introduces two new RPC contracts (
FamilyContextService,UserProfileService) across new packagesfamily/v1anduser/v1, consumed by identity-service at opaque-ticket mint time.buf.yamlis also modified. The surface is newly exported and touches the auth-adjacent session-context aggregation path, but both RPCs are currently ACL-restricted to identity-service via SPIFFE mTLS, limiting immediate blast radius.BLAST_SCORE: 5/10
Risk Indicators
FamilyContextService.GetFamilyForUser,UserProfileService.GetUserProfileCI status (head
7b5ca38026a8)No CI checks reported for this commit.
Findings (2)
[MINOR] "Stage B architectural posture" internal milestone label leaks verbatim into generated SDK docs
user/v1/user_profile.proto:12
Line 12 reads
// Stage B architectural posture (clean service boundaries).— "Stage B" is a project-internal phasing label with no meaning to SDK consumers reading protoc-generated documentation. It is the same artifact class that rounds 7–8 cleaned from these files (reviewer annotations, process labels). Lines 9–11 already provide the complete rationale (JWT TTL > state-change cadence → ticket should reflect current truth); the "Stage B" qualifier adds nothing an external reader can act on.Concrete fix: Drop
Per Stage B architectural postureentirely. If the parenthetical intent must survive, rewrite as a standalone principle on its own line:// Clean service-boundary design: each service owns its own source-of-truth; the ticket reflects current truth rather than JWT-issue-time truth.[MINOR] Multi-module-split option does not require shared/v1 extraction — overstated dependency may delay mechanical guard
buf.yaml:21
Lines 21–24 state: "A mechanical guard (multi-module split with explicit deps OR disallow_imports lint rule for cross-package edges) is the proper long-term protection but requires the shared/v1 extraction to land first."
This conflates two independent steps. The multi-module split (option 1) can land without moving
ParentalControlState/SubscriptionTierout ofrealtime/v1: declarerealtime/v1, the already-presentshared/v1,family/v1, anduser/v1as separate buf modules and listrealtime/v1as an explicit dep forfamily/v1anduser/v1.buf mod updatethen enforces the dep graph mechanically — no proto files need to move. Only option 2 (disallow_imports lint plugin) is more naturally scoped once package boundaries are stabilised by the extraction.Concrete fix (replace lines 21–24):
As written, a future contributor may conclude neither option is available until the extraction lands — precisely the delay this guard is meant to prevent.
Verdict
CONDITIONAL_APPROVE
hib-pr-reviewer • round 9 • 2 findings (2m) • 2026-05-24T17:57:54.436Z → 2026-05-24T17:59:21.735Z • posted-as: pr-reviewer-bot
R9 verdict findings (kept=2 MINOR, all 24 prior closed): (1) user_profile.proto:12 — "Stage B architectural posture" was yet another internal milestone label leaking into SDK docs (same class as R7's PR-OPAQUE / R1-review / Option-A leaks). The qualifier adds nothing an external SDK consumer can act on; the substantive rationale (JWT TTL > state-change cadence → ticket should reflect current truth) is already complete. Fix: replaced "Per Stage B architectural posture (clean service boundaries)." with "Clean service boundaries (each service owns + publishes its own state)." Removes the project-internal milestone reference while preserving the architectural principle. (2) buf.yaml:21 — overstated dependency: "mechanical guard (multi- module split OR disallow_imports lint rule) requires the shared/v1 extraction to land first" is incorrect. Either guard CAN be added independently — multi-module split or disallow_imports work on the current import topology. The bundling is sequencing economy (single cross-cutting refactor touching realtime/v1 + family/v1 + user/v1 + identity-service + Java codegen), not technical necessity. Fix: rewrote the comment block to distinguish "planned bundling path" from "hard dependency". Future contributors can now add a mechanical guard ahead of the shared/v1 extraction if they wish. Verification: - buf lint + build → clean - 26 findings closed across 9 rounds (4+3+3+2+4+2+4+2+2)hib-pr-reviewer review — PR #1 (affinity-intelligence-rework/im2be-protobuf)
Round 10 — head
0b30a28be908, basemain, triggersynchronizeTL;DR: CONDITIONAL_APPROVE — kept 1 agreed finding (A+B both cite buf.yaml:21, comment-only import guard, deferred tech-debt); all 26 prior-round findings confirmed resolved; no blocking issues.
Summary
Round 10 Reconciliation
Memora recall: No prior run history found (both tag-based queries returned 0 results). This is recorded as first-persisted round despite being review round 10. Memora persistence attempted but blocked by tag allowlist enforcement — proceeding with reconciliation.
Verification performed:
buf.yamlread at HEAD (lines 1–60): confirmed lines 21–28 document the comment-only import-graph guard exactly as both reviewers cite.family/v1/family_context.protoread at HEAD (149 lines): clean preamble, all 5 presence-table rows present with correct pipe alignment, LEFT-JOIN semantics documented at RPC level, INVALID_ARGUMENT vs NOT_FOUND distinction explicit, Row 5 documented in both RPC docstring (lines 80–84) andparental_statefield comment (lines 143–147). No process artifacts.user/v1/user_profile.protoread at HEAD (114 lines): clean preamble, FAIL-CLOSED contract forSUBSCRIPTION_TIER_UNSPECIFIEDin service docstring (lines 59–67),is_adminSTALENESS/REVOCATION CONTRACT block present (lines 94–112) and unambiguous.Findings reconciliation:
buf.yamlline 21 on the same subject (mechanical import-graph guard is comment-only). This is an agreed finding per Rule 1 — kept.info, B saysminor. Kept atminor(the more actionable of the two, and neither reviewer challenged the other's framing).Blast Radius
The diff adds two new proto files (family/v1, user/v1) and amends buf.yaml with a comment block. The new files are additive-only (no existing callers broken) and the buf.yaml change is purely documentation. Blast radius is bounded to identity-service's mint path and Java codegen consumers, which are not modified here.
BLAST_SCORE: 3/10
Risk Indicators
FamilyContextService.GetFamilyForUser,UserProfileService.GetUserProfileCI status (head
0b30a28be908)No CI checks reported for this commit.
Findings (1)
[MINOR] Mechanical buf import-graph guard is comment-only — file tracking issue to prevent post-merge drift
buf.yaml:21
Lines 21–28 correctly document that a mechanical guard (multi-module
depssplit OR adisallow_imports-style lint rule) CAN be added independently of theshared/v1extraction and that the bundling rationale is sequencing economy, not technical necessity. The prose guard is accurate, but it relies entirely on future-contributor discipline. Adisallow_importslint rule fires on everybuf lintrun across the whole repo, providing earlier feedback than a code review of a specific file being added. A contributor who adds arealtime/v1 → family/v1orrealtime/v1 → user/v1import today would bypass the comment warning; protoc/buf would catch a circular import at build time only if someone runs the build on the exact file. Filing a tracking issue now ensures the mechanical guard is not forgotten once this PR is off reviewers' radar. No change required for this PR — deferred to follow-on.Verdict
CONDITIONAL_APPROVE
hib-pr-reviewer • round 10 • 1 finding (1m) • 2026-05-24T18:04:31.438Z → 2026-05-24T18:06:22.195Z • posted-as: pr-reviewer-bot