feat(identity-resolution): remaining domains — roles, person-roles, visibility, subchart, internal lookup (#1755)

[mozhaev-dev]

What

Ports the remaining .NET identity endpoints to the Rust identity-resolution
service, completing code-level parity with the .NET service. Stacked on the
read (#1745) and write-seed (#1825) work, both already in main.

Endpoints added in this PR:

• Roles catalogue — POST/GET/DELETE /v1/roles (admin-gated; hard-delete with
  an atomic in-use guard).
• Person-roles — POST/GET/DELETE /v1/person-roles (grant/list/revoke;
  last-admin lockout guard enforced in a RepeatableRead txn with FOR UPDATE).
• Visibility — POST/GET/DELETE /v1/visibility (create/list/revoke grants).
• Org subchart — GET /v1/subchart (forest) and GET /v1/subchart/{person_id}
  (single), recursive-CTE traversals with the first visibility gating in the port.
• Internal lookup — GET /internal/persons/by-email/{email}, service-only
  (sub_type=service), for the authenticator's login bootstrap.
• Shared admin gate (require_admin / require_caller), and a bind_named
  helper that expands the .NET named @params to SeaORM positional ? so the
  recursive CTEs stay character-identical to the .NET source.

Verification

• 53 unit tests pass; clippy -D warnings clean.
• Full-branch adversarial review (multiple independent passes) + fixes:
  last-admin lockout race (txn + FOR UPDATE), reason length validation,
  driver-agnostic visibility probe, depth/field/limit hardening, bounded subchart
  depth + dropped-row telemetry.
• Endpoint-parity + consumer audit vs .NET. Analytics (POST /v1/profiles,
  forwards the user JWT) is compatible and now correctly visibility-gated. The
  authenticator's internal lookup is contract-matched but is currently blocked
  at the OIDC layer by an empty-tenant service token — see Known limitations.
• POST /v1/profiles visibility gate restored (was missing on the read path):
  require_caller + is_target_in_visible_set, deny→404, parity with the .NET
  VisibilityService.CanSeeAsync.
• Two rounds of adversarial review + CodeRabbit + two external reviews addressed
  (last-admin lock, reason validation, RFC-3339 valid_from, concurrent-dup 409,
  bounded subchart depth, per-field validation, service-gate test, …).

Intentional divergences from .NET (unchanged from #1745/#1825)

• Error-type namespace gts.cf.insight.identity_resolution.* (RFC-9457) instead
  of urn:insight:error:*.
• No HTTP 422 in the gears error model → ambiguous-profile → 409 aborted;
  role-in-use / last-admin → 400 failed_precondition.
• next_cursor dropped from list responses (was always null).
• Internal by-email route is a raw axum route, out of OpenAPI (mirrors the .NET
  .ExcludeFromDescription()).
• Tenant/caller come from the gateway JWT; X-Insight-Tenant-Id /
  X-Insight-Person-Id headers are no longer read.
• Self-managed SeaORM pool + raw SQL (not the toolkit db capability) — see
  toolkit-db: no raw-SQL path (DbConn/DbTx) — window functions, recursive CTEs & atomic conditional DML can't be expressed, forcing a bespoke SeaORM pool gears-rust#4239.
• depth on subchart is clamped to the server's max_depth (and defaults to it
  when omitted) — a deliberate bound the .NET endpoint lacked, so a caller cannot
  force an unbounded org-tree traversal.
• The deprecated GET /v1/persons/{email} is intentionally not carried: no
  consumer calls it, and the redesign removes it.

Known limitations — NOT yet swap-ready (do not switch traffic on merge)

This PR is code-only; the following block a production cutover and are
tracked for the next PR (#1753):

• Authenticator login blocked: it mints the service JWT with an empty
  tenant_id; the gears OIDC plugin parses tenant_id as a UUID and 401s before
  the handler, so GET /internal/persons/by-email/{email} is unreachable. Needs
  a tenantless service-token path (or a sentinel tenant) — a cross-service change.
• Migrations + bootstrap admin: the service only connects to MariaDB; it does
  not run DbUp or seed the first admin role. Migrations (incl. 013) still live
  in the .NET project. Clean-env bring-up + first-admin bootstrap are unresolved.
• Deployment: no Dockerfile/Helm/compose; the service binds 8083 while
  consumers expect identity:8082; env prefix changed (IDENTITY__ →
  APP__gears__identity-resolution__config__); OIDC full-auth config/CA not
  mounted.
• CI + contract: identity-resolution is not registered in CI
  (scripts/ci/components.py); the OpenAPI drift gate still targets .NET; there
  is no HTTP+MariaDB integration / parity suite. OpenAPI path/query params for the
  new routes are not yet declared (pairs with wiring the Rust drift gate).

Known parity quirks (faithful to .NET, documented)

• GET /v1/subchart/{person_id} synthesizes a null-valued root for an unknown id
  instead of 404 — the anchor is literal in Sql.Subchart.cs; unchanged here.
• GET /v1/persons/{email} (deprecated) intentionally dropped — no consumer.

Deferred polish

Helper de-duplication, a domain-owned subchart DTO, redacted Debug on the
ClickHouse password.

Part of #1755. Merging this does not enable the cutover — it lands the code;
the swap happens in #1753 after the blockers above are closed.

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features
  • Added admin APIs for managing roles, person-role assignments, and visibility grants.
  • Added authenticated subchart endpoints for visible forests and depth-bounded subtrees (with valid_at filtering).
  • Added an internal service-only endpoint to resolve a person by email.
• Bug Fixes
  • Profile resolution and subchart results now consistently respect visibility permissions.
  • Improved limit handling and strengthened protection against revoking the last active administrator.
• Documentation
  • Updated identity-resolution documentation for the expanded, authenticated API surface.
• Tests
  • Added coverage for validation and internal response formatting.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: c9add4f8-0f75-4fd6-9c73-7cbe36789812

📥 Commits

Reviewing files that changed from the base of the PR and between a3a82dc and d6c53d8.

📒 Files selected for processing (3)

• src/backend/services/identity-resolution/src/api/datetime.rs
• src/backend/services/identity-resolution/src/api/handlers.rs
• src/backend/services/identity-resolution/src/api/subchart.rs

🚧 Files skipped from review as they are similar to previous changes (2)

• src/backend/services/identity-resolution/src/api/datetime.rs
• src/backend/services/identity-resolution/src/api/handlers.rs

---

📝 Walkthrough

Walkthrough

The PR expands identity-resolution with shared authorization, internal email lookup, role and visibility management APIs, recursive subchart queries, hierarchical response assembly, canonical errors, repository support, and route registration.

Changes

Identity-resolution API expansion

Layer / File(s)	Summary
Authorization, lookup, and route wiring src/backend/services/identity-resolution/src/api/{error,gate,handlers,mod,seed,datetime}.rs, src/backend/services/identity-resolution/src/infra/db/persons_repo.rs, src/backend/services/identity/src/Insight.Identity.Infrastructure/Migrations/013_persons_email_any_tenant_idx.sql, src/backend/services/identity-resolution/README.md, src/backend/services/identity-resolution/src/main.rs	Adds shared caller/admin/service authorization, canonical errors, service-only email lookup, visibility checks, seed-handler reuse, flexible datetime handling, authentication wiring, and routes for the new operations.
Roles and person-role lifecycle src/backend/services/identity-resolution/src/api/{roles,person_roles}.rs, src/backend/services/identity-resolution/src/infra/db/{roles_repo,person_roles_repo}.rs	Adds admin-gated role CRUD and person-role grant/list/revoke operations with validation, tenant scoping, soft deletion, and last-admin protection.
Visibility grant lifecycle src/backend/services/identity-resolution/src/api/visibility.rs, src/backend/services/identity-resolution/src/infra/db/visibility_repo.rs	Adds visibility grant creation, filtering, listing, and soft revocation with bounded parameters and canonical responses.
Subchart retrieval and assembly src/backend/services/identity-resolution/src/api/subchart.rs, src/backend/services/identity-resolution/src/domain/{mod,subchart}.rs, src/backend/services/identity-resolution/src/infra/db/{mod,sql_named,subchart_repo}.rs	Adds recursive visibility-aware subchart queries, named SQL binding, depth and timestamp validation, and flat-row-to-tree response assembly with tests.

Estimated code review effort: 4 (Complex) | ~60 minutes

Possibly related issues

• identity-resolution: port remaining domains + cutover #1755 — The PR implements the subchart, roles, person-roles, and visibility endpoints described by the issue.
• GRAPH 1: Identity resolution — port the C# service to Rust #1602 — The PR advances the Rust identity-resolution service port with the remaining APIs, authorization, and service wiring.

Possibly related PRs

• constructorfabric/insight#1745 — Both PRs modify the identity-resolution API, including profile handling and canonical errors.
• constructorfabric/insight#1825 — Both PRs update persons-seed authorization and limit parsing around the shared admin gate.

Sequence Diagram(s)

sequenceDiagram
  participant Client
  participant SubchartAPI
  participant SubchartRepo
  participant DomainAssembler
  Client->>SubchartAPI: Request subchart with depth and valid_at
  SubchartAPI->>SubchartRepo: Query visible hierarchy using recursive CTEs
  SubchartRepo-->>SubchartAPI: Return flat subchart nodes
  SubchartAPI->>DomainAssembler: Assemble forest or subtree
  DomainAssembler-->>SubchartAPI: Return hierarchical response
  SubchartAPI-->>Client: Return subchart JSON

Loading

Suggested reviewers: mitasovr

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main identity-resolution expansion and names the newly added domains and internal lookup.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/identity-resolution-domains-cutover

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 3

🧹 Nitpick comments (3)

src/backend/services/identity-resolution/src/infra/db/person_roles_repo.rs (1)

185-259: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win

Guarded lock/update runs for every revoke, not only admin-role revokes — avoidable lock contention.

try_soft_delete_protecting_last_admin always takes a FOR UPDATE lock on every active-admin row in the tenant (Lines 194-204), even when the assignment being revoked isn't the admin role. Since the caller (person_roles.rs::delete_person_role) already fetched the target row and knows its role_id before calling this function, non-admin revokes could skip the lock/guard path entirely and go straight to a plain soft-delete, avoiding contention with concurrent admin-roster changes in busy tenants.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/backend/services/identity-resolution/src/infra/db/person_roles_repo.rs`
around lines 185 - 259, The try_soft_delete_protecting_last_admin flow
unnecessarily locks the tenant’s active-admin rows for non-admin revocations.
Use the caller’s known target role_id to branch in delete_person_role: perform
the guarded lock/count/update only for admin-role revokes, and use the existing
plain soft-delete path for other roles; preserve the current last-admin
protection and reason update behavior.

src/backend/services/identity-resolution/src/api/handlers.rs (1)

95-132: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a test for the service-principal gate.

Only the wire-shape of InternalPersonResponse is tested; the security-critical ctx.subject_type() != Some("service") → 403 branch (Line 108) has no test asserting a non-service caller is rejected. Since this is a SERVICE-ONLY login-bootstrap endpoint, a regression here would be high-impact and silent.

Also applies to: 367-393

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/backend/services/identity-resolution/src/api/handlers.rs` around lines 95
- 132, The internal_person_by_email handler lacks coverage for rejecting
non-service principals. Add a test that invokes the handler with a
SecurityContext whose subject_type is not "service" and asserts it produces the
canonical 403 permission-denied response, while preserving the existing
successful response-shape test.

src/backend/services/identity-resolution/src/api/gate.rs (1)

41-58: 🔒 Security & Privacy | 🔵 Trivial

Consider logging denied admin-gate attempts.

Only the DB-error branch logs (tracing::error!); the 403 "not admin" branch is silent. Since this gate protects every admin-only endpoint (roles, person-roles, visibility, persons-seed), a log/metric on denial would help detect repeated unauthorized probing.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/backend/services/identity-resolution/src/api/gate.rs` around lines 41 -
58, Add a warning log or metric in require_admin for the !is_admin denial path,
including the caller and tenant context when available, while preserving the
existing permission_denied response and DB-error logging.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/backend/services/identity-resolution/src/api/roles.rs`:
- Around line 70-93: Trim req.name before passing it to role_name_valid,
roles_repo::get_by_name, and the subsequent role creation/storage flow. Use the
trimmed value consistently for duplicate detection and persistence while
preserving the existing validation and error behavior.

In `@src/backend/services/identity-resolution/src/api/visibility.rs`:
- Around line 109-120: Update the visibility request validation near the
existing viewer_person_id check to reject Some(viewed_person_id) when that UUID
is nil, using the same invalid-argument field-violation pattern and appropriate
viewed_person_id field name. Preserve None as the whole-tree visibility case and
leave reason_valid validation unchanged.

In `@src/backend/services/identity-resolution/src/infra/db/persons_repo.rs`:
- Around line 80-115: Add a tenant-agnostic composite index for the lookup
performed by resolve_person_id_by_email_any_tenant, covering value_type and
value_id and preferably the created_at, id, and person_id columns used for
ranking and selection. Define it in the persons table schema or migration using
the project’s existing index conventions.

---

Nitpick comments:
In `@src/backend/services/identity-resolution/src/api/gate.rs`:
- Around line 41-58: Add a warning log or metric in require_admin for the
!is_admin denial path, including the caller and tenant context when available,
while preserving the existing permission_denied response and DB-error logging.

In `@src/backend/services/identity-resolution/src/api/handlers.rs`:
- Around line 95-132: The internal_person_by_email handler lacks coverage for
rejecting non-service principals. Add a test that invokes the handler with a
SecurityContext whose subject_type is not "service" and asserts it produces the
canonical 403 permission-denied response, while preserving the existing
successful response-shape test.

In `@src/backend/services/identity-resolution/src/infra/db/person_roles_repo.rs`:
- Around line 185-259: The try_soft_delete_protecting_last_admin flow
unnecessarily locks the tenant’s active-admin rows for non-admin revocations.
Use the caller’s known target role_id to branch in delete_person_role: perform
the guarded lock/count/update only for admin-role revokes, and use the existing
plain soft-delete path for other roles; preserve the current last-admin
protection and reason update behavior.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 082ea410-0ca6-4251-bbbf-20ffd4d35e7c

📥 Commits

Reviewing files that changed from the base of the PR and between bcd79be and 071325c.

📒 Files selected for processing (18)

• src/backend/services/identity-resolution/src/api/error.rs
• src/backend/services/identity-resolution/src/api/gate.rs
• src/backend/services/identity-resolution/src/api/handlers.rs
• src/backend/services/identity-resolution/src/api/mod.rs
• src/backend/services/identity-resolution/src/api/person_roles.rs
• src/backend/services/identity-resolution/src/api/roles.rs
• src/backend/services/identity-resolution/src/api/seed.rs
• src/backend/services/identity-resolution/src/api/subchart.rs
• src/backend/services/identity-resolution/src/api/visibility.rs
• src/backend/services/identity-resolution/src/domain/mod.rs
• src/backend/services/identity-resolution/src/domain/subchart.rs
• src/backend/services/identity-resolution/src/infra/db/mod.rs
• src/backend/services/identity-resolution/src/infra/db/person_roles_repo.rs
• src/backend/services/identity-resolution/src/infra/db/persons_repo.rs
• src/backend/services/identity-resolution/src/infra/db/roles_repo.rs
• src/backend/services/identity-resolution/src/infra/db/sql_named.rs
• src/backend/services/identity-resolution/src/infra/db/subchart_repo.rs
• src/backend/services/identity-resolution/src/infra/db/visibility_repo.rs

[coderabbitai]

🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Look for migration/DDL defining the `persons` table and its indexes.
fd -i migration
rg -n --type sql -i -A3 -B3 'CREATE TABLE .*persons|CREATE (UNIQUE )?INDEX' 
rg -n -i -A5 'persons' -g '*migration*' -g '*.sql'

Repository: constructorfabric/insight

Length of output: 565

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "## candidates"
git ls-files | rg -n 'persons|migration|schema|migrations|migrat|db' | sed -n '1,200p'

echo "## persons references in tracked files"
rg -n --hidden -i '\bpersons\b' | sed -n '1,240p'

echo "## SQL DDL/INDEX references"
rg -n --hidden -i -C 2 'CREATE TABLE|CREATE UNIQUE INDEX|CREATE INDEX|INDEX|UNIQUE KEY|KEY|ALTER TABLE' --glob '*.sql' --glob '*.cs' --glob '*.rs' --glob '*.rs.inu' --glob '*.sql.*' --glob '*.fs' --glob '*.fsx' | sed -n '1,260p'

Repository: constructorfabric/insight

Length of output: 16231

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo "## persons migration SQL"
sed -n '1,240p' src/backend/services/identity/src/Insight.Identity.Infrastructure/Migrations/001_persons.sql
echo "---"
sed -n '1,240p' src/backend/services/identity/src/Insight.Identity.Infrastructure/Migrations/004_persons_relax_constraints.sql

echo "## entity/model"
sed -n '1,220p' src/backend/services/identity-resolution/src/infra/db/entities/persons.rs

echo "## local repository references"
rg -n -i '\bpersons\b|value_type|value_id|insight_tenant_id|create_unique_index|create_index|person_id' src/backend/services/identity src/backend/services/identity-resolution src/backend/services/analytics --glob '*.sql' --glob '*.rs' --glob '*.cs' | sed -n '1,360p'

echo "## migrations index DDL"
rg -n --type sql -C 3 'person|idx|index|unique|persons' src/backend/services/identity/src/Insight.Identity.Infrastructure/Migrations src/backend/services/identity-resolution src/backend/services/analytics/src/migration --glob '*.sql' --glob '*.rs' | sed -n '1,420p'

Repository: constructorfabric/insight

Length of output: 50382

---

Add a matching non-tenant index for tenant-agnostic email resolution.

persons only has tenant-prefixed indexes (insight_tenant_id, value_type, value_id) plus value columns, so resolve_person_id_by_email_any_tenant’s CTE cannot use those indexes for its value_type = 'email' AND value_id = ? filter. Add an index covering this lookup pattern, e.g. (value_type, value_id, created_at, id, person_id) or tenant-agnostic indexes on the value columns used in the CTE/ORDER BY.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/backend/services/identity-resolution/src/infra/db/persons_repo.rs` around
lines 80 - 115, Add a tenant-agnostic composite index for the lookup performed
by resolve_person_id_by_email_any_tenant, covering value_type and value_id and
preferably the created_at, id, and person_id columns used for ranking and
selection. Define it in the persons table schema or migration using the
project’s existing index conventions.

[coderabbitai]

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/backend/services/identity-resolution/src/api/handlers.rs (1)

135-152: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Reject ambiguous email matches before resolving identities.

resolve_person_id_by_email_any_tenant ranks one latest row per (tenant, source, value_id) and then returns a single row across tenants. Duplicate normalized emails across tenants can therefore return the biggest/newest source-id match and avoid signaling ambiguity; detect multiple matched persons before returning one person_id.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/backend/services/identity-resolution/src/api/handlers.rs` around lines
135 - 152, Update the by-email lookup around
resolve_person_id_by_email_any_tenant to retrieve and validate all matching
persons before selecting an identity, rejecting the request when multiple
distinct person matches exist rather than accepting the ranked result. Preserve
the existing internal error mapping, not-found response, and successful
InternalPersonResponse behavior for exactly one match.

🧹 Nitpick comments (1)

src/backend/services/identity/src/Insight.Identity.Infrastructure/Migrations/013_persons_email_any_tenant_idx.sql (1)

8-9: 🚀 Performance & Scalability | 🔵 Trivial

Verify online-DDL behavior before applying this index to production.

persons is likely a hot table, and index creation can acquire metadata or table locks depending on the database version and engine. Confirm the target migration strategy supports online DDL or schedule this migration outside peak traffic; Squawk’s PostgreSQL-style CONCURRENTLY suggestion should not be copied blindly to MariaDB/MySQL.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In
`@src/backend/services/identity/src/Insight.Identity.Infrastructure/Migrations/013_persons_email_any_tenant_idx.sql`
around lines 8 - 9, Verify the production database engine/version and migration
strategy for the idx_value_id_any_tenant index before applying it. Ensure CREATE
INDEX in migration 013_persons_email_any_tenant_idx.sql uses the supported
online-DDL approach for MariaDB/MySQL, or explicitly schedule it outside peak
traffic; do not add PostgreSQL-specific CONCURRENTLY syntax.

Source: Linters/SAST tools

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@src/backend/services/identity-resolution/src/api/handlers.rs`:
- Around line 41-71: The visibility check currently runs only after ambiguity
handling, leaking hidden matches and rejecting a uniquely visible candidate.
Update the `resolve_person_ids` flow to filter all candidate IDs through
`subchart_repo::is_target_in_visible_set` before matching on the results,
preserving the existing error mapping and 404 behavior for no visible
candidates; then perform the one-result versus ambiguity decision on the
filtered set.

---

Outside diff comments:
In `@src/backend/services/identity-resolution/src/api/handlers.rs`:
- Around line 135-152: Update the by-email lookup around
resolve_person_id_by_email_any_tenant to retrieve and validate all matching
persons before selecting an identity, rejecting the request when multiple
distinct person matches exist rather than accepting the ranked result. Preserve
the existing internal error mapping, not-found response, and successful
InternalPersonResponse behavior for exactly one match.

---

Nitpick comments:
In
`@src/backend/services/identity/src/Insight.Identity.Infrastructure/Migrations/013_persons_email_any_tenant_idx.sql`:
- Around line 8-9: Verify the production database engine/version and migration
strategy for the idx_value_id_any_tenant index before applying it. Ensure CREATE
INDEX in migration 013_persons_email_any_tenant_idx.sql uses the supported
online-DDL approach for MariaDB/MySQL, or explicitly schedule it outside peak
traffic; do not add PostgreSQL-specific CONCURRENTLY syntax.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: f699fc58-c90d-4569-8aee-bbe505c73c8a

📥 Commits

Reviewing files that changed from the base of the PR and between 071325c and 71eed53.

📒 Files selected for processing (9)

• src/backend/services/identity-resolution/src/api/gate.rs
• src/backend/services/identity-resolution/src/api/handlers.rs
• src/backend/services/identity-resolution/src/api/person_roles.rs
• src/backend/services/identity-resolution/src/api/roles.rs
• src/backend/services/identity-resolution/src/api/subchart.rs
• src/backend/services/identity-resolution/src/api/visibility.rs
• src/backend/services/identity-resolution/src/infra/db/person_roles_repo.rs
• src/backend/services/identity-resolution/src/infra/db/subchart_repo.rs
• src/backend/services/identity/src/Insight.Identity.Infrastructure/Migrations/013_persons_email_any_tenant_idx.sql

🚧 Files skipped from review as they are similar to previous changes (7)

• src/backend/services/identity-resolution/src/infra/db/subchart_repo.rs
• src/backend/services/identity-resolution/src/infra/db/person_roles_repo.rs
• src/backend/services/identity-resolution/src/api/gate.rs
• src/backend/services/identity-resolution/src/api/roles.rs
• src/backend/services/identity-resolution/src/api/person_roles.rs
• src/backend/services/identity-resolution/src/api/subchart.rs
• src/backend/services/identity-resolution/src/api/visibility.rs

[coderabbitai]

🧹 Nitpick comments (1)

src/backend/services/identity-resolution/src/api/datetime.rs (1)

54-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Test the serde adapter at the deserialization boundary.

parse_flexible is covered, but deserialize_opt adds distinct behavior for missing, empty, and invalid values. Add a small serde_json-backed test matrix for these cases.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/backend/services/identity-resolution/src/api/datetime.rs` around lines 54
- 88, The existing tests only cover parse_flexible and not the deserialize_opt
serde boundary. Add a concise serde_json-backed test matrix in the tests module
that exercises missing, empty, valid, and invalid values, asserting the expected
optional datetime results and deserialization errors for deserialize_opt.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@src/backend/services/identity-resolution/src/api/datetime.rs`:
- Around line 54-88: The existing tests only cover parse_flexible and not the
deserialize_opt serde boundary. Add a concise serde_json-backed test matrix in
the tests module that exercises missing, empty, valid, and invalid values,
asserting the expected optional datetime results and deserialization errors for
deserialize_opt.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 6096952e-4625-48bf-b033-619e5468e85f

📥 Commits

Reviewing files that changed from the base of the PR and between 71eed53 and a3a82dc.

📒 Files selected for processing (21)

• src/backend/services/identity-resolution/README.md
• src/backend/services/identity-resolution/src/api/datetime.rs
• src/backend/services/identity-resolution/src/api/gate.rs
• src/backend/services/identity-resolution/src/api/handlers.rs
• src/backend/services/identity-resolution/src/api/mod.rs
• src/backend/services/identity-resolution/src/api/person_roles.rs
• src/backend/services/identity-resolution/src/api/roles.rs
• src/backend/services/identity-resolution/src/api/seed.rs
• src/backend/services/identity-resolution/src/api/visibility.rs
• src/backend/services/identity-resolution/src/domain/seed.rs
• src/backend/services/identity-resolution/src/domain/seed_service.rs
• src/backend/services/identity-resolution/src/infra/db/entities/account_person_map.rs
• src/backend/services/identity-resolution/src/infra/db/entities/mod.rs
• src/backend/services/identity-resolution/src/infra/db/ops_repo.rs
• src/backend/services/identity-resolution/src/infra/db/person_roles_repo.rs
• src/backend/services/identity-resolution/src/infra/db/persons_repo.rs
• src/backend/services/identity-resolution/src/infra/db/roles_repo.rs
• src/backend/services/identity-resolution/src/infra/db/seed_repo.rs
• src/backend/services/identity-resolution/src/infra/db/visibility_repo.rs
• src/backend/services/identity-resolution/src/infra/identity_inputs.rs
• src/backend/services/identity-resolution/src/main.rs

💤 Files with no reviewable changes (10)

• src/backend/services/identity-resolution/src/domain/seed_service.rs
• src/backend/services/identity-resolution/src/infra/db/entities/account_person_map.rs
• src/backend/services/identity-resolution/src/infra/db/entities/mod.rs
• src/backend/services/identity-resolution/src/infra/identity_inputs.rs
• src/backend/services/identity-resolution/src/infra/db/seed_repo.rs
• src/backend/services/identity-resolution/src/domain/seed.rs
• src/backend/services/identity-resolution/src/infra/db/ops_repo.rs
• src/backend/services/identity-resolution/src/infra/db/visibility_repo.rs
• src/backend/services/identity-resolution/src/infra/db/person_roles_repo.rs
• src/backend/services/identity-resolution/src/infra/db/roles_repo.rs

🚧 Files skipped from review as they are similar to previous changes (7)

• src/backend/services/identity-resolution/src/infra/db/persons_repo.rs
• src/backend/services/identity-resolution/src/api/gate.rs
• src/backend/services/identity-resolution/src/api/visibility.rs
• src/backend/services/identity-resolution/src/api/mod.rs
• src/backend/services/identity-resolution/src/api/handlers.rs
• src/backend/services/identity-resolution/src/api/roles.rs
• src/backend/services/identity-resolution/src/api/person_roles.rs

[cyberantonz]

Some finding over code