feat(identity-resolution): cutover readiness — parity fixes, side-by-side deploy, schema ownership (#1602) - #1918
Merged
Conversation
…1550) Port the .NET hotfix 3256f70 to the Rust reader so both services read identity_inputs identically. The dbt producer writes insight_tenant_id hashed (sipHash128 of the free-form connector tenant string), so a plain `= tenant` predicate never matches the caller's tenant and the seed silently reads 0 rows. Deployments are single-tenant, so the tenant predicate is dropped entirely; the seed still writes its output under the caller's tenant (run_seed binds the request tenant, never the row's). The IdentityInputsReader trait keeps the tenant_id parameter so the filter can come back once the tenant representation is unified end to end (multi-tenant prerequisite, documented inline). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Sergei Mozhaev <mozhaev.dev@gmail.com>
Parity with .NET, whose reader.GetString() throws on a NULL source_account_id and fails the seed run. The Rust reader folded NULL into '' via ifNull(), silently minting a '' pseudo-account instead of surfacing the bad producer row. Decode as Option<String> and error with the row context (source_type/source_id/value_type). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Sergei Mozhaev <mozhaev.dev@gmail.com>
Migration 009 aligned persons/org_chart to DATETIME(6) but left account_person_map on TIMESTAMP(6), which is session-timezone-dependent: the .NET connection (server tz) and the Rust identity-resolution pool (pinned time_zone='+00:00') would read the same stored instant as different wall-clock values whenever the server tz is not UTC, skewing SCD2 valid_from/valid_to comparisons across the two services during the cutover. The migration pins the session tz to UTC so the TIMESTAMP -> DATETIME conversion renders stored instants as UTC literals regardless of the server default. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Sergei Mozhaev <mozhaev.dev@gmail.com>
… service Consumers (gateway route, analytics/authenticator identity_url) reach identity at <host>:8082, so keeping the port means the dev cutover flips only the hostname in three umbrella values, and rollback reverts them. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Sergei Mozhaev <mozhaev.dev@gmail.com>
Both modeled on the analytics service (the gears-rust template): - Dockerfile: src/backend build context, workspace-manifest dependency caching, shared docker-entrypoint.sh, non-root runtime, EXPOSE 8082. - Helm chart insight-identity-resolution: ConfigMap-rendered gears config with the oidc-authn-plugin verification block (trusted_issuers / audience / custom CA paths — the Rust host resolves JWKS via https OIDC discovery, so the authenticator CA Secret is mounted at /etc/insight/authn-ca, unlike the .NET service), envFrom Secret for the APP__gears__identity-resolution__config__* leaf overrides, wait-for-mariadb initContainer (copied from the .NET identity chart), /health + /healthz probes. No migrate initContainer: the .NET service still owns the DbUp schema during the transition. The chart is deployed side-by-side with the .NET identity service and receives no traffic until the umbrella flips the gateway/consumer URLs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Sergei Mozhaev <mozhaev.dev@gmail.com>
- dependency insight-identity-resolution gated on identityResolution.deploy (off by default) — deployed alongside the .NET identity service during the cutover; traffic stays on .NET until the gateway/consumer URLs flip. - identityResolution values block: image, resources, gateway-JWT verification wiring (issuer FQDN + authn-tls CA Secret, same as analytics), databaseName pointing at the SAME `identity` MariaDB database the .NET service owns and migrates. - secrets.yaml: composes insight-identity-resolution-config with the APP__gears__identity-resolution__config__* leaf overrides (MariaDB DSN, ClickHouse over HTTP — not the native port the .NET service uses). - mariadb-init-svcdbs-job: provision the identity DB when EITHER identity service deploys. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Sergei Mozhaev <mozhaev.dev@gmail.com>
- docker-compose.yml: identity-resolution service (internal 8082, host 8086 — the .NET identity keeps host 8082), bind-mounted binary + full-auth config overlay + authn-tls CA, same pattern as analytics. - deploy/compose/identity-resolution-fullauth.yaml: oidc-authn-plugin wired to the authn-tls discovery front with the self-signed CA. - dev-compose.sh: identity-resolution in the backend list, rust build batch, --from-ghcr/IDENTITY_RESOLUTION_IMAGE flip, and `build` targets. - .env.compose.example: IDENTITY_RESOLUTION_IMAGE / _PORT knobs. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Sergei Mozhaev <mozhaev.dev@gmail.com>
…e image - components.py: rust entry (fmt/clippy/tests gate the pipeline; cover=False mirroring authenticator/identity — the business logic is exercised by env-gated live tests that skip in CI; re-enable with the #1753 integration suite). - build-images.yml: identity_resolution change filter, backend-identity-resolution/merge-identity-resolution job pair (multi-arch, push-by-digest, provenance attestation — mirrors analytics), publish-chart appVersion bump + umbrella appVersion max input, bump-descriptors/publish-chart gating. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Sergei Mozhaev <mozhaev.dev@gmail.com>
- environments/local/values.yaml.template: identityResolution.deploy=true — the Rust pod runs next to the .NET identity service, no traffic routed (also applied to the operator's gitignored values.yaml copy). - compose-app-secrets.sh: compose insight-identity-resolution-config (APP__gears__identity-resolution__config__* — same identity MariaDB database, ClickHouse over HTTP). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Sergei Mozhaev <mozhaev.dev@gmail.com>
cargo fmt --check gates the CI rust job; the fetch_all chain from the NULL-handling change was not fmt-clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Sergei Mozhaev <mozhaev.dev@gmail.com>
…lock Review findings on the cutover-prep branch: - mariadb-init-svcdbs-job gated only on identity.deploy: a Rust-only install (identityResolution.deploy=true, identity.deploy=false) skipped DB provisioning entirely and would boot against a database that neither exists nor has schema (the Rust service runs no DDL — DbUp in the .NET service owns it). The render now FAILS on a Rust-only install, FAILS when the two databaseName values diverge, and the job renders when either service deploys. Drop the guards when identity-resolution gains its own migrate step. - Chart.lock: regenerated with the insight-identity-resolution dependency (was only updated locally, breaking helm dependency build reproducibility). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Sergei Mozhaev <mozhaev.dev@gmail.com>
The analytics and identity-resolution images compile the insight-clickhouse path dependency in, but their build-images change filters watched only the service directory + workspace manifests — a lib-only change could merge without republishing either image. Add the lib path to both filters, and to the identity-resolution component paths so changed.py re-runs its tests on lib changes too. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Sergei Mozhaev <mozhaev.dev@gmail.com>
Review follow-up: component_for() resolves each changed path to a SINGLE owner, so listing src/backend/libs/insight-clickhouse under identity-resolution's paths never fired — the lib path always resolves to the insight-clickhouse component and identity-resolution only joined the matrix through the lint fanout (test=false). Use the registry's triggered_by co-trigger instead (same mechanism as the connector harness/mock pairing): a lib change now puts identity-resolution in the matrix with its tests on. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Sergei Mozhaev <mozhaev.dev@gmail.com>
…ate subcommand, bootstrap admin Schema-ownership transfer (epic #1602): the .NET identity service is frozen and will be decommissioned once the Rust service is validated, so schema migrations move here, uniform with the analytics service (sea-orm-migration + `migrate` CLI subcommand, run by a Helm initContainer). - src/migration/: all 14 DbUp scripts as individual sea-orm migrations. The sql/ files are byte-for-byte copies of the .NET DbUp scripts (review parity with `diff -r`); a tiny splitter strips `--` comments and feeds statements one by one (statement counts pinned by a unit test). Every script is idempotent, so the first run on an environment DbUp already migrated is a no-op sweep that only fills the seaql_migrations ledger; DbUp's SchemaVersions is left orphaned until decommission. Rollback policy until then: additive-only. - `migrate` subcommand (clap Option<Commands>, no subcommand = server): connects with a single-connection pool, serializes concurrent migrators via a GET_LOCK advisory lock (session-scoped), applies pending migrations, then runs the first-admin bootstrap. - infra/db/bootstrap.rs: port of the .NET BootstrapAdminRunner — idempotent INSERT … WHERE NOT EXISTS of an active admin assignment for (tenant_default_id, bootstrap_admin_person_id); same skip semantics (no person → silent, no tenant → warn). New gear config fields tenant_default_id / bootstrap_admin_person_id. Verified against a throwaway MariaDB 11: fresh DB → 14 applied, all tables + admin role + bootstrap admin present; re-run → 0 applied, bootstrap not duplicated; run over a schema applied DbUp-style (the 14 .NET files via mariadb client) → clean no-op sweep; SHOW CREATE TABLE of all 7 tables byte-identical between the two paths. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Sergei Mozhaev <mozhaev.dev@gmail.com>
…r wiring - identity-resolution chart: migrate initContainer (analytics pattern) applying migrations + first-admin bootstrap before the server pod; runs regardless of waitForMariadb. - umbrella: Rust-only installs are now allowed (the initContainer brings a fresh database up from scratch) — the render-fail guard is reduced to the databaseName-equality check while both services deploy; mariadb-init provisions the DB from whichever identity service is enabled. New identityResolution.bootstrapAdminPersonId value + optional tenant_default_id / bootstrap_admin_person_id env vars in insight-identity-resolution-config (from global.tenantDefaultId, mirroring the .NET block). - gitops: compose-app-secrets.sh emits the same two optional fields; local values template comment updated (Rust owns the schema). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Sergei Mozhaev <mozhaev.dev@gmail.com>
…t-authored Drop 014_account_person_map_datetime.sql from the .NET DbUp set (added earlier on this branch, never deployed anywhere): the Rust migrate initContainer now owns the schema, so the DATETIME(6) conversion ships only as the Rust-side migration. One applier for new DDL — no concurrent-ALTER window with the .NET startup DbUp, and the frozen-.NET invariant stays literal (its journal and script set never change again). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Sergei Mozhaev <mozhaev.dev@gmail.com>
…, compose migrate, CI live-db suite Review findings on the schema-ownership step: - 012_org_chart_nullable_parent: DROP/ADD CONSTRAINT are now IF EXISTS / IF NOT EXISTS guarded (the one deliberate edit in the copied script set, documented in the file and mod.rs). MariaDB DDL is not transactional and the ledger row lands only after the whole script — a migrator killed between DROP and ADD left every re-run failing on "Can't DROP CONSTRAINT". The guards also make each statement safe against the frozen .NET DbUp replaying its own 012 on a fresh side-by-side install; that residual (transient .NET pod restart on a brand-new DB, converging) is documented in the umbrella values. - docker-compose: one-shot identity-resolution-migrate service; the server gates on service_completed_successfully. Without it the compose stack only got DbUp 001-013 and never the Rust-authored 014. - compose-app-secrets.sh: the Rust DSN was built from identity's databaseName — a Rust-only install with a custom identityResolution.databaseName provisioned one database and migrated another. New IDENTITY_RESOLUTION_DB with fallback. - CI now exercises the migrator on MariaDB: identity-resolution is live_db=true (new live_db_name registry field — the provisioned database is `identity`, not the analytics default); the rust job's no-coverage path applies migrations via the CLI up front (env-passed override — the gear name contains a dash) and the new live test re-runs the migrator (idempotency), replays the 012 kill-window (constraint restored), and double-runs the bootstrap (single active admin). Verified locally against MariaDB 11.4. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Sergei Mozhaev <mozhaev.dev@gmail.com>
The generated override switched only the identity-resolution server to the ghcr image; the one-shot identity-resolution-migrate service kept its build + local-binary bind mount. In ghcr mode that binary is intentionally not built, so the migrate container could never start and the server blocked forever on service_completed_successfully (and on Apple silicon the amd64 platform pin was missing). The companion now gets its own override — build reset, volumes cleared (the image's baked config supplies /app/config/insight.yaml), platform pinned — while the base command (… migrate) is preserved. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Sergei Mozhaev <mozhaev.dev@gmail.com>
…ire parity Two consumer-visible incompatibilities the identity contract suite (#1753, PR #1897) catches by design: - role_in_use and last_admin_protected were `failed_precondition` (400); every other .NET-422 guard maps to `aborted` (409 — ambiguous_profile set the precedent, gears has no 422). Both now use the same mapping, keeping the guard reason and detail text; the suite pins the family as UNPROCESSABLE_OR_CONFLICT = {422, 409}. - the four list responses (roles, person-roles, visibility, persons-seed) lost the .NET ListResponse's `next_cursor` field in the #1755 refactor. Restored as an always-null declared field — pagination is not implemented in either implementation, but the envelope is the wire contract consumers parse (the suite's strict list_response asserts both keys). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Sergei Mozhaev <mozhaev.dev@gmail.com>
- SubchartNode.subordinates lacked #[schema(no_recursion)] — utoipa's schema generation recursed forever and the server ABORTED with a main-thread stack overflow at route registration (PersonResponse had the guard, the subchart tree did not). The service could not boot at all with docs/schema registration on. - the identity_inputs reader decoded `toString(insight_source_id)` into a strict String, but the dbt column is Nullable(String) and toString of a Nullable stays Nullable — the clickhouse decoder rejected the mismatch and every seed failed with a schema error. Wrapped in ifNull: a NULL becomes '' and fails the UUID reparse, failing the seed exactly like the .NET reader's Guid.Parse(GetString(...)) throw. Both found by the first E2E_IDENTITY_IMPLEMENTATION=rust run of the identity contract suite (#1753). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Sergei Mozhaev <mozhaev.dev@gmail.com>
- runner image bakes identity-resolution (build-only compose service from its own Dockerfile + COPY, same pattern as analytics) — the E2E_IDENTITY_IMPLEMENTATION=rust run needs nothing else. - persons-seed end-to-end now verifies the seed through the actual read contract: freshly minted person_ids come from the tenant-agnostic internal by-email lookup (the seed admin has no org_chart presence, so /v1/profiles correctly answers 404 for them — visibility semantics the dotnet skip had left unexercised); the same-email collapse is proven by resolving AS the minted person (a top-of-tree sees itself), with ids[]/by-id asserting the CURRENT-id-per-source reduction both implementations share. Fixture rows carry distinct _synced_at (production reality — the persons UNIQUE key silently drops same-instant collisions) and per-account `id` observations (connectors emit them; ids[] is built from exactly those). Parity proof: E2E_IDENTITY_IMPLEMENTATION=rust → 71 passed + 2 skipped (the dotnet-only deprecated lookup), gate PASS (identity-rust suite); the SAME suite vs dotnet → 72 passed + 1 skipped (the containerized-CH seed e2e), gate PASS 18/18 at 100%. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Sergei Mozhaev <mozhaev.dev@gmail.com>
…p under lock - docker-compose: identity-resolution-migrate joins *backend-common — it was on the default network, could not resolve `mariadb`, and the server would block forever on service_completed_successfully. - CI runs the identity contract suite against BOTH implementations: the existing lane stays on dotnet (the deployed implementation, gated until decommission) and a new e2e-identity-rust lane + identity-rust-endpoint-coverage-gate (suite identity-rust — the dropped legacy endpoint is an approved SKIP there) run the successor; both fold into the required Run E2E suite check. The e2e cache overlay gains gha scopes for the identity + identity-resolution build-only images so PR runs don't cold-compile them. - the first-admin bootstrap now runs INSIDE the migration advisory lock: nothing constrains the active (tenant, person, role) triple, so two replicas' initContainers racing after the lock release could insert two bootstrap assignments. run_migrations takes the gear config and owns the whole critical section; the live test covers the combined path. - build-images: the shared docker-entrypoint.sh joins the identity_resolution image filter (it is baked into the image). - migration 014 wrapper doc no longer claims a .NET origin — it is the first Rust-authored migration (matches migration/mod.rs). Verified: rust suite 71 passed + 2 skipped, dotnet suite 72 passed + 1 skipped, both gates PASS; clippy/fmt/compose config clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Sergei Mozhaev <mozhaev.dev@gmail.com>
… count Review follow-ups: - The migration advisory lock serializes RUST migrators only — the frozen .NET BootstrapAdminRunner takes no lock, so with BOTH services configured to bootstrap an admin, a cross-service race could insert two active assignments (nothing constrains the active (tenant, person, role) triple). Ownership is now enforced at the config level: the umbrella render FAILS when identity.bootstrapAdmin- PersonId and identityResolution.bootstrapAdminPersonId are both set while both services deploy. The lock comment no longer overclaims cross-service serialization. - The live test counted reason='bootstrap' rows GLOBALLY, so a legitimate bootstrap for a different tenant/person in the same database failed it. The count is scoped to the test's exact (tenant, person, ADMIN_ROLE_ID) triple. Verified: dual-bootstrap render fails, single-owner render passes; live migrations+bootstrap test green on a fresh MariaDB 11.4; clippy/fmt clean, 58 crate tests pass. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Sergei Mozhaev <mozhaev.dev@gmail.com>
Final self-review sweep: both still claimed the .NET DbUp owns the identity schema — the ownership transferred to the Rust migrator earlier on this branch (migration/mod.rs is the authority). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Sergei Mozhaev <mozhaev.dev@gmail.com>
|
Warning Review limit reached
Next review available in: 54 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdds the Rust identity-resolution service with MariaDB migrations, first-admin bootstrap, API parity updates, local and Kubernetes deployment support, image/chart publishing, CI database handling, and dedicated Rust E2E coverage. ChangesIdentity Resolution Service
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
.github/workflows/build-images.yml (1)
1122-1127: 🎯 Functional Correctness | 🔴 Critical | ⚡ Quick winIdentity-resolution's Helm chart appVersion bump is computed but never published.
Three related gaps prevent the umbrella chart from ever picking up the identity-resolution image tag:
- Line 1200-1208:
publish-chart's trigger OR-condition lists analytics/authenticator/gateway/identity/toolbox/umbrella/frontend_tag, but never checksneeds.changes.outputs.identity_resolution == 'true'. If a PR only touches identity-resolution source,publish-chartis skipped entirely — the umbrella chart is never republished with the new image ref.- Line 1124-1127 (
bump-descriptors"Commit descriptor patches" step): thegit addlist stages analytics/authenticator/gateway/identity Chart.yaml files but omitssrc/backend/services/identity-resolution/helm/Chart.yaml, even though the preceding step (1094-1103) already runsbump-service-appversions.shwithIDENTITY_RESOLUTIONwired in and modifies that file.- Line 1421-1429 (
publish-chart"Commit version bumps back to main" step): samegit addomission —identity-resolution/helm/Chart.yamlis bumped by the script (1273-1282) but never staged.The existing
identityentries in bothgit addlists (and the if-list) confirm this is the established pattern that identity-resolution should also follow — its absence looks like an oversight rather than intentional scoping.🐛 Proposed fix
&& ( needs.changes.outputs.analytics == 'true' || needs.changes.outputs.authenticator == 'true' || needs.changes.outputs.gateway == 'true' || needs.changes.outputs.identity == 'true' + || needs.changes.outputs.identity_resolution == 'true' || needs.changes.outputs.toolbox == 'true' || needs.changes.outputs.umbrella == 'true' || inputs.frontend_tag != '' )git add src/backend/services/analytics/helm/Chart.yaml \ src/backend/services/authenticator/helm/Chart.yaml \ src/backend/services/gateway/helm/Chart.yaml \ - src/backend/services/identity/helm/Chart.yaml + src/backend/services/identity/helm/Chart.yaml \ + src/backend/services/identity-resolution/helm/Chart.yaml(apply the same addition to the
git addlist in the "Commit version bumps back to main" step at line 1421-1429, plussrc/frontend/helm/Chart.yamlalready present there)Also applies to: 1200-1208, 1421-1429
🤖 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 @.github/workflows/build-images.yml around lines 1122 - 1127, Update the workflow’s identity-resolution handling in the publish-chart trigger condition and both “git add” steps: include needs.changes.outputs.identity_resolution == 'true' in the publish condition, and stage src/backend/services/identity-resolution/helm/Chart.yaml alongside the existing backend service charts in “Commit descriptor patches” and “Commit version bumps back to main.”src/backend/services/identity-resolution/src/main.rs (1)
44-50: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMake the config option work with the documented migration invocation.
The README documents
identity-resolution --config config/insight.yaml, butMigrateparses--configas a parent-only option, soidentity-resolution migrate --config ...is rejected. Either makeconfiga global argument or document/reconcile this to requireidentity-resolution --config ... migrate.Suggested fix
- #[arg(short, long)] + #[arg(short, long, global = true)]🤖 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/main.rs` around lines 44 - 50, Update the Cli config argument so it is accepted by subcommands, including the documented identity-resolution migrate --config invocation; mark the existing config field in Cli as a global argument while preserving its optional PathBuf behavior and current command parsing.
🧹 Nitpick comments (2)
.github/workflows/ci.yml (1)
188-221: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCoverage step's migrate call still hardcodes the
analyticsgear name.Line 206 hardcodes
APP__gears__analytics__config__database_url, unlike the new "Test (no coverage)" step below (lines 227-246) which correctly parametrizes on${{ matrix.entry.name }}. This is harmless today since analytics is the onlycover=true && live_db=trueentry, but components.py's own comment for identity-resolution flags plans to re-enable coverage (#1753) — at that point this hardcoded value would silently break the migrate step for identity-resolution's Coverage run.♻️ Proposed fix — mirror the new step's parametrization
- APP__gears__analytics__config__database_url="$INTEGRATION_TESTS_MARIADB_URL" \ + env "APP__gears__${{ matrix.entry.name }}__config__database_url=$INTEGRATION_TESTS_MARIADB_URL" \ cargo llvm-cov run --no-report --package "${{ matrix.entry.package }}" $feats -- \ -c "services/${{ matrix.entry.name }}/config/insight.yaml" migrate🤖 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 @.github/workflows/ci.yml around lines 188 - 221, Parameterize the migration environment variable in the “Coverage” step using the component name from matrix.entry.name instead of the hardcoded analytics gear key. Keep the existing database URL assignment and migrate command unchanged, matching the parametrization already used by the “Test (no coverage)” step..github/workflows/e2e-bronze-to-api.yml (1)
317-317: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winDisable credential persistence on the two new checkout steps.
Both new jobs (
e2e-identity-rust,identity-rust-endpoint-coverage-gate) check out the repo withoutpersist-credentials: false, flagged by zizmor's artipacked rule — theGITHUB_TOKENremains in the git config for the rest of the job. This mirrors the pre-existing pattern elsewhere in the file, but since these are new jobs it's a good point to start hardening.
.github/workflows/e2e-bronze-to-api.yml#L317-L317: addwith: persist-credentials: falseto theactions/checkout@v4step ine2e-identity-rust..github/workflows/e2e-bronze-to-api.yml#L571-L571: addwith: persist-credentials: falseto theactions/checkout@v4step inidentity-rust-endpoint-coverage-gate.🤖 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 @.github/workflows/e2e-bronze-to-api.yml at line 317, Disable credential persistence on both new checkout steps: in .github/workflows/e2e-bronze-to-api.yml lines 317-317 for e2e-identity-rust and lines 571-571 for identity-rust-endpoint-coverage-gate, add the checkout step’s persist-credentials: false setting under with.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 @.github/workflows/build-images.yml:
- Around line 73-77: Add src/backend/docker-entrypoint.sh to the analytics path
filter in the workflow alongside the existing analytics paths, ensuring changes
to the shared entrypoint trigger analytics image builds.
- Around line 617-619: Update the checkout step in this job to set
persist-credentials to false, matching the hardened checkout convention used
elsewhere in the workflow. Leave the following docker/setup-buildx-action step
unchanged.
In @.github/workflows/ci.yml:
- Around line 172-176: Update the MariaDB startup step’s condition to require
both the component’s live_db flag and an actual DB-consuming run, using the
matrix cover or test flags so lint-only fanout runs do not start a container
while preserving support for test=true, cover=false identity-resolution runs.
In `@src/backend/services/identity-resolution/helm/values.yaml`:
- Around line 80-92: Swap the HTTP probe paths in the Helm values: set
livenessProbe to use /healthz for process liveness and readinessProbe to use
/health for database readiness. Leave the existing probe timing and ports
unchanged.
In `@src/backend/services/identity-resolution/src/api/seed.rs`:
- Around line 156-159: The capped list endpoints currently return no
continuation cursor, preventing pagination. Update the persons-seed handlers in
seed.rs at lines 156-159 and 300-303 and the visibility handlers in
visibility.rs at lines 82-85 and 192-195 to use a deterministic ordering and
cursor, returning next_cursor when the page is filled; otherwise omit it or use
an unbounded query as appropriate.
In `@src/backend/services/identity-resolution/src/migration/sql/007_roles.sql`:
- Around line 21-24: Update the admin seed migration to validate that the
existing admin role matches both the canonical role_id and name. Do not allow
the current duplicate-key no-op to succeed when name='admin' has a different
role_id; fail the migration or safely reconcile the row so the canonical admin
UUID is present, while preserving successful idempotent execution for the
correct pair.
In
`@src/backend/services/identity-resolution/src/migration/sql/011_operations.sql`:
- Around line 6-16: Update the operations table definition to preserve queued
operations without a start timestamp: add a non-null created_at column with the
current UTC default, and make started_at nullable without a default so it is
populated only when the operation transitions to running.
---
Outside diff comments:
In @.github/workflows/build-images.yml:
- Around line 1122-1127: Update the workflow’s identity-resolution handling in
the publish-chart trigger condition and both “git add” steps: include
needs.changes.outputs.identity_resolution == 'true' in the publish condition,
and stage src/backend/services/identity-resolution/helm/Chart.yaml alongside the
existing backend service charts in “Commit descriptor patches” and “Commit
version bumps back to main.”
In `@src/backend/services/identity-resolution/src/main.rs`:
- Around line 44-50: Update the Cli config argument so it is accepted by
subcommands, including the documented identity-resolution migrate --config
invocation; mark the existing config field in Cli as a global argument while
preserving its optional PathBuf behavior and current command parsing.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 188-221: Parameterize the migration environment variable in the
“Coverage” step using the component name from matrix.entry.name instead of the
hardcoded analytics gear key. Keep the existing database URL assignment and
migrate command unchanged, matching the parametrization already used by the
“Test (no coverage)” step.
In @.github/workflows/e2e-bronze-to-api.yml:
- Line 317: Disable credential persistence on both new checkout steps: in
.github/workflows/e2e-bronze-to-api.yml lines 317-317 for e2e-identity-rust and
lines 571-571 for identity-rust-endpoint-coverage-gate, add the checkout step’s
persist-credentials: false setting under with.
🪄 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: c04b3d13-df1c-4ae5-86bd-0d4e045503dd
⛔ Files ignored due to path filters (2)
charts/insight/Chart.lockis excluded by!**/*.locksrc/backend/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (70)
.env.compose.example.github/workflows/build-images.yml.github/workflows/ci.yml.github/workflows/e2e-bronze-to-api.yml.github/workflows/scripts/bump-service-appversions.shcharts/insight/Chart.yamlcharts/insight/templates/mariadb-init-svcdbs-job.yamlcharts/insight/templates/secrets.yamlcharts/insight/values.yamldeploy/compose/identity-resolution-fullauth.yamldeploy/gitops/environments/local/values.yaml.templatedeploy/gitops/scripts/compose-app-secrets.shdev-compose.shdocker-compose.ymlscripts/ci/changed.pyscripts/ci/components.pysrc/backend/services/identity-resolution/Cargo.tomlsrc/backend/services/identity-resolution/Dockerfilesrc/backend/services/identity-resolution/README.mdsrc/backend/services/identity-resolution/config/insight.yamlsrc/backend/services/identity-resolution/helm/Chart.yamlsrc/backend/services/identity-resolution/helm/templates/_helpers.tplsrc/backend/services/identity-resolution/helm/templates/configmap.yamlsrc/backend/services/identity-resolution/helm/templates/deployment.yamlsrc/backend/services/identity-resolution/helm/templates/service.yamlsrc/backend/services/identity-resolution/helm/values.yamlsrc/backend/services/identity-resolution/src/api/person_roles.rssrc/backend/services/identity-resolution/src/api/roles.rssrc/backend/services/identity-resolution/src/api/seed.rssrc/backend/services/identity-resolution/src/api/visibility.rssrc/backend/services/identity-resolution/src/config.rssrc/backend/services/identity-resolution/src/domain/subchart.rssrc/backend/services/identity-resolution/src/gear.rssrc/backend/services/identity-resolution/src/infra/db/bootstrap.rssrc/backend/services/identity-resolution/src/infra/db/mod.rssrc/backend/services/identity-resolution/src/infra/identity_inputs.rssrc/backend/services/identity-resolution/src/main.rssrc/backend/services/identity-resolution/src/migration/m20260724_000001_persons.rssrc/backend/services/identity-resolution/src/migration/m20260724_000002_account_person_map.rssrc/backend/services/identity-resolution/src/migration/m20260724_000003_org_chart.rssrc/backend/services/identity-resolution/src/migration/m20260724_000004_persons_relax_constraints.rssrc/backend/services/identity-resolution/src/migration/m20260724_000005_tighten_source_type.rssrc/backend/services/identity-resolution/src/migration/m20260724_000006_visibility.rssrc/backend/services/identity-resolution/src/migration/m20260724_000007_roles.rssrc/backend/services/identity-resolution/src/migration/m20260724_000008_person_roles.rssrc/backend/services/identity-resolution/src/migration/m20260724_000009_align_existing_tables_to_conventions.rssrc/backend/services/identity-resolution/src/migration/m20260724_000010_account_person_map_idx_by_account.rssrc/backend/services/identity-resolution/src/migration/m20260724_000011_operations.rssrc/backend/services/identity-resolution/src/migration/m20260724_000012_org_chart_nullable_parent.rssrc/backend/services/identity-resolution/src/migration/m20260724_000013_persons_email_any_tenant_idx.rssrc/backend/services/identity-resolution/src/migration/m20260724_000014_account_person_map_datetime.rssrc/backend/services/identity-resolution/src/migration/mod.rssrc/backend/services/identity-resolution/src/migration/sql/001_persons.sqlsrc/backend/services/identity-resolution/src/migration/sql/002_account_person_map.sqlsrc/backend/services/identity-resolution/src/migration/sql/003_org_chart.sqlsrc/backend/services/identity-resolution/src/migration/sql/004_persons_relax_constraints.sqlsrc/backend/services/identity-resolution/src/migration/sql/005_tighten_source_type.sqlsrc/backend/services/identity-resolution/src/migration/sql/006_visibility.sqlsrc/backend/services/identity-resolution/src/migration/sql/007_roles.sqlsrc/backend/services/identity-resolution/src/migration/sql/008_person_roles.sqlsrc/backend/services/identity-resolution/src/migration/sql/009_align_existing_tables_to_conventions.sqlsrc/backend/services/identity-resolution/src/migration/sql/010_account_person_map_idx_by_account.sqlsrc/backend/services/identity-resolution/src/migration/sql/011_operations.sqlsrc/backend/services/identity-resolution/src/migration/sql/012_org_chart_nullable_parent.sqlsrc/backend/services/identity-resolution/src/migration/sql/013_persons_email_any_tenant_idx.sqlsrc/backend/services/identity-resolution/src/migration/sql/014_account_person_map_datetime.sqlsrc/ingestion/tests/e2e/compose/Dockerfile.runnersrc/ingestion/tests/e2e/compose/docker-compose.cache.ymlsrc/ingestion/tests/e2e/compose/docker-compose.runner.ymlsrc/ingestion/tests/e2e/identity/test_persons_seed.py
Comment on lines
+156
to
+159
| /// Wire parity with the .NET `ListResponse`: the cursor is declared | ||
| /// but pagination is not implemented — always `null` (both | ||
| /// implementations return every row; consumers already tolerate it). | ||
| pub next_cursor: Option<String>, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
git ls-files | rg 'src/backend/services/identity-resolution/src/api/(seed|visibility)\.rs$' || true
echo
echo "Outlines:"
ast-grep outline src/backend/services/identity-resolution/src/api/seed.rs --match PersonsSeedListResponse --view expanded || true
ast-grep outline src/backend/services/identity-resolution/src/api/visibility.rs --match VisibilityListResponse --view expanded || true
echo
echo "Relevant seed.rs lines 130-200, 270-315:"
sed -n '130,200p;270,315p' src/backend/services/identity-resolution/src/api/seed.rs
echo
echo "Relevant visibility.rs lines 50-105, 165-205:"
sed -n '50,105p;165,205p' src/backend/services/identity-resolution/src/api/visibility.rsRepository: constructorfabric/insight
Length of output: 8562
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate ops_repo and visibility_repo list implementations:"
rg -n "mod ops_repo|pub mod ops_repo|mod visibility_repo|pub mod visibility_repo|pub async fn list|pub fn list" src/backend/services/identity-resolution/src -S
echo
echo "Files under relevant parent dirs:"
fd . src/backend/services/identity-resolution/src -t f | rg 'ops_repo|visibility_repo|repository|repo' || true
echo
echo "Search list methods in seed/visibility directories:"
rg -n "async fn list|fn list|limit" src/backend/services/identity-resolution/src/api src/backend/services/identity-resolution/src -S
echo
echo "Search ops_repo::list definitions/usages:"
rg -n "ops_repo::list|list\\(&, \?|pub async fn list|pub fn list" src/backend -S || true
echo
echo "Search visibility_repo::list definitions/usages:"
rg -n "visibility_repo::list|list\\(&, \?|pub async fn list|pub fn list" src/backend -S || trueRepository: constructorfabric/insight
Length of output: 20133
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "ops_repo.rs list implementation:"
sed -n '195,235p' src/backend/services/identity-resolution/src/infra/db/ops_repo.rs
echo
echo "visibility_repo.rs list implementation:"
sed -n '90,122p' src/backend/services/identity-resolution/src/infra/db/visibility_repo.rs
echo
echo "Behavioral/static probe: bounded list APIs always return next_cursor None and use LIMIT from query params"
python3 - <<'PY'
from pathlib import Path
import re
def read(path): return open(path, encoding="utf-8").read()
paths = [
Path("src/backend/services/identity-resolution/src/api/seed.rs"),
Path("src/backend/services/identity-resolution/src/infra/db/ops_repo.rs"),
Path("src/backend/services/identity-resolution/src/api/visibility.rs"),
Path("src/backend/services/identity-resolution/src/infra/db/visibility_repo.rs"),
]
data = {p: read(p) for p in paths}
checks = [
("seed handler passes limit to ops_repo::list",
"ops_repo::list(&state.db, tenant, Some(PERSONS_SEED_OP), status, limit)" in data[paths[0]]),
("seed repo appends LIMIT ?",
bool(re.search(r"ORDER BY started_at DESC, operation_id DESC LIMIT \\\?", data[paths[1]]))),
("seed response sets next_cursor None",
"NextCursor::None, items," in data[paths[0]] or "next_cursor: None," in data[paths[0]] and "PersonListResponse {\\n items," not in data[paths[0]]),
("visibility handler passes limit to visibility_repo::list",
"visibility_repo::list(\n &state.db,\n tenant,\n params.viewer,\n params.viewed,\n params.active.unwrap_or(false),\n limit,\n )" in data[paths[2]]),
("visibility repo appends LIMIT ?",
bool(re.search(r"ORDER BY created_at DESC, visibility_id DESC LIMIT \\\?", data[paths[3]]))),
("visibility response sets next_cursor None",
"next_cursor: None," in data[paths[2]]),
]
for name, ok in checks:
print(f"{name}: {'PASS' if ok else 'FAIL'}")
print("summary:", all(x[1] for x in checks))
PYRepository: constructorfabric/insight
Length of output: 3269
Return continuation cursors from capped list queries.
GET /v1/persons-seed and GET /v1/visibility both pass the requested limit into a bounded SQL query (LIMIT ?) but reply with next_cursor: None, so clients cannot retrieve rows beyond the page. Implement a deterministic cursor and return it when a page is filled; otherwise drop next_cursor / unbounded the query.
📍 Affects 2 files
src/backend/services/identity-resolution/src/api/seed.rs#L156-L159(this comment)src/backend/services/identity-resolution/src/api/seed.rs#L300-L303src/backend/services/identity-resolution/src/api/visibility.rs#L82-L85src/backend/services/identity-resolution/src/api/visibility.rs#L192-L195
🤖 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/seed.rs` around lines 156 -
159, The capped list endpoints currently return no continuation cursor,
preventing pagination. Update the persons-seed handlers in seed.rs at lines
156-159 and 300-303 and the visibility handlers in visibility.rs at lines 82-85
and 192-195 to use a deterministic ordering and cursor, returning next_cursor
when the page is filled; otherwise omit it or use an unbounded query as
appropriate.
Comment on lines
+21
to
+24
| -- Admin seed; the constant is mirrored by Domain.Services.Roles.Admin. | ||
| INSERT INTO roles (role_id, name) | ||
| VALUES (UNHEX('a4d11000000040008000000000000001'), 'admin') | ||
| ON DUPLICATE KEY UPDATE name = name; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Fail closed on seeded-admin identity conflicts.
roles.rs resolves admin privileges using the canonical admin UUID. If an existing row already has name = 'admin' but a different role_id, this ON DUPLICATE KEY clause becomes a no-op and leaves the canonical UUID absent while the migration succeeds. Validate the (role_id, name) pair and fail or safely reconcile conflicting existing data.
🤖 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/migration/sql/007_roles.sql`
around lines 21 - 24, Update the admin seed migration to validate that the
existing admin role matches both the canonical role_id and name. Do not allow
the current duplicate-key no-op to succeed when name='admin' has a different
role_id; fail the migration or safely reconcile the row so the canonical admin
UUID is present, while preserving successful idempotent execution for the
correct pair.
Comment on lines
+6
to
+16
| CREATE TABLE IF NOT EXISTS operations ( | ||
| operation_id BINARY(16) NOT NULL, | ||
| operation_type VARCHAR(64) NOT NULL, | ||
| status VARCHAR(16) NOT NULL, | ||
| insight_tenant_id BINARY(16) NOT NULL, | ||
| author_person_id BINARY(16) NOT NULL, | ||
| request_json JSON NULL, | ||
| summary_json JSON NULL, | ||
| error_message TEXT NULL, | ||
| started_at DATETIME(6) NOT NULL DEFAULT (UTC_TIMESTAMP(6)), | ||
| completed_at DATETIME(6) NULL, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Keep started_at unset while an operation is queued.
The documented lifecycle includes queued → running, but started_at is NOT NULL DEFAULT (UTC_TIMESTAMP(6)), so inserting a queued row immediately records it as started. Add a separate created_at and populate nullable started_at only when transitioning to running, or remove the queued state.
🤖 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/migration/sql/011_operations.sql`
around lines 6 - 16, Update the operations table definition to preserve queued
operations without a start timestamp: add a non-null created_at column with the
current UTC default, and make started_at nullable without a default so it is
populated only when the operation transitions to running.
…emantics - build-images: the shared docker-entrypoint.sh joins the analytics image filter too (its Dockerfile bakes it in, same as identity-resolution's). - backend-identity-resolution checkout sets persist-credentials: false. - ci.yml: the MariaDB container starts only when the matrix entry actually runs tests (live_db && (cover || test)) — a lint-only fanout entry no longer pays the ~15s startup. - identity-resolution probes follow the endpoint semantics: /healthz (cheap process ping) for liveness, /health (the deeper status endpoint) for readiness — so an unhealthy pod leaves rotation instead of waiting for a liveness restart. Declined with reasons (PR comment): real pagination cursors (the always-null next_cursor is documented .NET wire parity — neither implementation paginates; post-decommission follow-up), and edits to 007_roles.sql / 011_operations.sql (byte-for-byte copies of the frozen .NET DbUp scripts — the diff -r parity invariant; behavior matches the .NET service by design until decommission). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Sergei Mozhaev <mozhaev.dev@gmail.com>
Contributor
Author
|
Addressed the CodeRabbit review in 81efe25: Fixed (4):
Declined with reasons (3):
|
mitasovr
approved these changes
Jul 25, 2026
mozhaev-dev
enabled auto-merge
July 25, 2026 06:59
cyberantonz
reviewed
Jul 27, 2026
| IDENTITY_DB=$(yq -r '.identity.databaseName // "identity"' "$VALUES") | ||
| TENANT_DEFAULT=$(yq -r '.global.tenantDefaultId // ""' "$VALUES") | ||
| IDENTITY_ORG_CHART_SOURCE=$(yq -r '.identity.orgChartSourceType // ""' "$VALUES") | ||
| IDENTITY_RESOLUTION_BOOTSTRAP_ADMIN=$(yq -r '.identityResolution.bootstrapAdminPersonId // ""' "$VALUES") |
Contributor
There was a problem hiding this comment.
This gitops is the sample, so all related changes should be reflected to local deploy gitops for dev/virtuozzo
cyberantonz
reviewed
Jul 27, 2026
| version: 0.1.0 | ||
| # appVersion is managed by the release pipeline (build-images.yml): it is | ||
| # bumped to the freshly built image tag on merges that change this service. | ||
| appVersion: "0.0.0-dev" |
Contributor
There was a problem hiding this comment.
Is it updated by bump-service-appversions.sh ?
cyberantonz
reviewed
Jul 27, 2026
| (include "insight.mariadb.port" .) | ||
| (required "identityResolution.databaseName is required" .Values.identityResolution.databaseName) | ||
| | quote }} | ||
| APP__gears__identity-resolution__config__clickhouse_url: {{ include "insight.clickhouse.url" . | quote }} |
Contributor
There was a problem hiding this comment.
The environment variables used to pass those values will definitely fail.
3 tasks
cyberantonz
pushed a commit
to cyberantonz/insight
that referenced
this pull request
Jul 27, 2026
The bump-descriptors and "commit version bumps back to main" steps in build-images.yml staged a hardcoded list of backend subchart Chart.yaml files that never included src/backend/services/identity-resolution/helm/Chart.yaml (bump-service- appversions.sh does bump it correctly in the run's workspace — it's just never git-added, so the bump is silently discarded). constructorfabric#1890 fixed the identical gap for authenticator/gateway three days earlier; identity-resolution landed as a subchart the next day (constructorfabric#1918) and missed the same two lists. Because the umbrella chart's appVersion computation already reads this file (line ~1345, unaffected), the chart still "worked" on any release that also rebuilt identity-resolution in the same CI run — the bump landed in the workspace and got packaged, just never persisted to main. The first release afterward that didn't touch identity-resolution's source packaged whatever was last committed: the original "0.0.0-dev" placeholder, which isn't a real GHCR tag. On dev (where identityResolution.deploy is now true post-cutover), that rendered an unpullable image, the migrate initContainer got stuck in ImagePullBackOff, and the umbrella upgrade failed on its progress deadline and auto-rolled back — repeatedly, since nothing fixed the root cause between attempts. One-time restore: set appVersion back to the last known-good tag (2026.07.25.07.16-a122f77 — verified currently running on dev) so the next release packages an existing image instead of the placeholder. Signed-off-by: Sergei Mozhaev <mozhaev.dev@gmail.com>
This was referenced Jul 28, 2026
This was referenced Aug 5, 2026
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.
What
Everything needed to swap the .NET identity service for the Rust
identity-resolutionon dev — except the traffic flip itself, which stays a separate 3-value change (gateway upstream + analytics/authenticatoridentity_url) with an equally small rollback. The .NET service keeps running side-by-side until decommission.Phase 0 — logic parity
identity_inputsread; trait keeps the parameter for the multi-tenant restore).source_account_idfails the seed with row context (parity with the .NET throw) instead of minting a''pseudo-account.Phase 1 — side-by-side deployability
identityResolution.deploy(default false — merging deploys nothing),insight-identity-resolution-configsecret composer, DB provisioning from either flag, render-fail guards (divergingdatabaseNames; dual bootstrap owners).live_dbMariaDB migration suite;triggered_byco-trigger on insight-clickhouse), build-images multi-arch image + chart publish wiring.Phase 2 — schema ownership transfer
sql/001…013are byte-for-byte copies of the frozen .NET scripts (review withdiff -r; one documented idempotency edit in 012), 014 is the first Rust-authored migration (deliberately absent from the .NET set — one applier for new DDL).migratesubcommand: single-connection session,GET_LOCKadvisory lock over migrations and the first-admin bootstrap (BootstrapAdminRunner port), run by the Helm migrate initContainer. Idempotent over an existing DbUp schema (verified: fresh-DB vs DbUp-appliedSHOW CREATE TABLEbyte-identical for all 7 tables; re-run = no-op; concurrent migrators serialize). Rollback policy until decommission: additive-only.Contract-suite driven fixes (the #1753 suite caught all of these)
role_in_use/last_admin_protected: 400 → 409/aborted (the same mapping asambiguous_profile; the suite pins the family as {422, 409}).next_cursorrestored in all four list DTOs (wire parity with the .NETListResponse).SubchartNode.subordinatesmissing#[schema(no_recursion)]— the server aborted with a stack overflow at route registration (could not boot at all).identity_inputsreader:ifNull(toString(insight_source_id))— the Nullable(String) column failed every seed against the strict decoder.Standing parity proof in CI
New
e2e-identity-rustlane + gate run the SAME contract suite against this implementation on every PR (the dotnet lane keeps gating the deployed service until decommission); both fold into the requiredRun E2E suitecheck.Verification
-D warnings, fmt. Migrations live-verified on MariaDB 11.4 incl. the 012 crash-recovery window and concurrent-migrator serialization (independently reproduced by review).After merge (separate steps)
identityResolution.deployon the dev overlay → pod runs side-by-side, no traffic.Refs #1602, #1753, #1755.
🤖 Generated with Claude Code
Summary by CodeRabbit