chore(personhog): extract persons module and router client to common - #73254
Merged
jose-sequeira merged 3 commits intoJul 24, 2026
Conversation
Replica, identity, and future personhog services each declared their own person row type, storage error enum, uuidv5 namespace, and router-calling conventions. Move them to personhog-common so services cannot drift: - persons: Person, StorageError/StorageResult, person_uuid(), proto conversion - client: RouterClient wrapping the generated PersonHogServiceClient, owning the leader-routing header contract (x-team-id/x-person-id, x-read-consistency) internally personhog-replica now re-exports the shared types; no behavior change.
jose-sequeira
force-pushed
the
jose-sequeira/personhog-common-persons-client
branch
from
July 23, 2026 14:42
f42d65a to
6937430
Compare
Contributor
Prompt To Fix All With AIFix the following 1 code review issue. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 1
rust/personhog-common/src/client.rs:84-91
**Routing branches lack regression tests**
This shared client is now the single owner of routing metadata, but its distinct branches lack focused tests verifying that strong reads and writes receive the required headers while eventual and unspecified reads do not. A later metadata change can silently violate the router contract without a focused test detecting it.
Reviews (1): Last reviewed commit: "chore(personhog): extract persons module..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
2 tasks
z0br0wn
reviewed
Jul 23, 2026
…nches Review feedback: StorageError classification moves from persons.rs to its own storage_error module, and the RouterClient's request builders gain parameterized tests pinning the header contract per routing branch — leader-bound calls (writes, strong reads) carry the routing headers, replica-bound reads carry none.
|
🔀 Tried to auto-resolve conflicts with I won't retry until the branch or master moves. |
nickbest-ph
approved these changes
Jul 23, 2026
jose-sequeira
enabled auto-merge (squash)
July 24, 2026 06:52
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
The personhog services are growing: replica reads person rows, the upcoming identity service creates them, and the lifecycle-manager from the identity saga RFC will follow. Each Rust service so far declared its own copy of the person row type, the storage error enum, the deterministic uuidv5 scheme, and the router-calling conventions (the
x-team-id/x-person-idleader-routing headers). Three copies of the same wire contract is how services drift apart.Changes
personhog_common::personsmodule: thePersonstorage row type,StorageError/StorageResult(with thesqlx::Errorconversion), thePERSON_UUIDV5_NAMESPACE+person_uuid()helper, and the storage-to-protoPersonconversion.personhog_common::clientmodule:RouterClient, a thin wrapper over the generatedPersonHogServiceClientthat owns the leader-routing contract internally. Leader-bound calls (update_person_properties, strongget_person) get the routing headers stamped from the request's own ids, so callers can't forget them. Eventual reads carry no headers, as before.personhog-replicanow re-exports the shared types from its existing module paths, so its call sites are unchanged. Its localFrom<sqlx::Error>impl, uuidv5 constant, and proto conversion are deleted in favor of the shared ones (the orphan rule enforces the single copy once the type lives in common).No behavior change and no wire change: the headers and types are today's contract, relocated.
How did you test this code?
Automated only, run by the agent in this branch's worktree:
cargo test -p personhog-common(54 tests) andcargo test -p personhog-replica(333 tests across suites, against the localposthog_personsdatabase)cargo build,cargo clippy --all-targets -- -D warnings,cargo shear,cargo fmt --checkon both cratesThe re-export approach means replica's storage/service code compiles unchanged, which is itself the regression check for the type move.
Automatic notifications
🤖 Agent context
Autonomy: Human-driven (agent-assisted)
Authored with Claude Code while building the personhog-identity service (follow-up PR, stacked on this). The identity work surfaced the duplication: identity needed the same
Persontype, error taxonomy, uuid scheme, and router headers that replica and the test harness had each declared locally. José directed the extraction: pool construction was already shared viacommon-database, so only the domain primitives and the client contract moved. A broader "move header constants only" variant was considered and rejected in favor of a fullRouterClientso the routing contract is owned by one type rather than re-implemented per caller. Skill used:/adding-personhog-rpc(for the surrounding identity work).