feat(analytics,identity): metric-results keys on person_id — the identity cutover (⚠️ merge together with the frontend) - #2098
Conversation
Iteration 2 of the metrics person_id rework: the runtime switches from the email entity key to the canonical person UUID the identity pipeline has been resolving into the gold tables since #2065/#2066. Hard switch by decision — no compatibility flag; rollout is gated on the dev coverage numbers, not on code. - validation: `entity.ids` parse as UUIDs into `person_ids: Vec<Uuid>` (parse-don't-validate; the pre-cutover email shape is now a loud 400, never a silent empty result). The email lowercasing normalizer is gone. - compiler: every view (period/timeseries incl. capped, breakdown, histogram, ranking, peer) filters `person_id IN (…)`, groups and orders by person_id, and projects `toString(assumeNotNull(person_id))`. Unresolved rows (person_id NULL) fall out of every query — the epic's "excluded, not guessed" stance, now enforced by the key itself. - peer: the cohort join runs person_id = person_id instead of string email equality — the fragile HR-email↔source-email match this rework exists to kill. The person grain is hardened with LIMIT 1 BY person_id in both cohort CTEs: the view is unique per EMAIL, and two emails resolving to one person must not double-weight a peer or blend pools. - schema validator: person_id joins OBSERVATION_COLUMNS/COHORT_COLUMNS in this same change, as the constants' comment promised — a table without the column now fails the probe loudly instead of answering empty. - naming discipline (review requirement): everything carrying the person UUID is named person_id/person_ids through validation, compiler rows, batch demux and builder; `entity_id` survives ONLY as the wire field name, documented as a seam at the single builder/DTO boundary. 501 unit tests green (peer-guard and ranking tests updated to the new grain), clippy pedantic clean, fmt clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Sergei Mozhaev <mozhaev.dev@gmail.com>
…ested cohorts, contract docs Review follow-ups on the person_id cutover (Codex): - schema validator: person_id actually joins OBSERVATION_COLUMNS / COHORT_COLUMNS now — the previous commit CLAIMED this but the edit had silently missed (the comment block drifted after the #2066 merge and the patch matched nothing). Without it a person_id-less table passes the probe and every query 500s at SQL time; with it the source is marked unavailable upfront. - peer cohorts: contested membership is EXCLUDED, not tie-broken. The LIMIT 1 BY person_id survivor pick silently awarded a person with conflicting cohort rows (two emails, two departments) to the lexicographically first cohort — against the epic's never-silently- assign stance. Both cohort CTEs now collapse to person grain with GROUP BY + HAVING uniqExact(cohort_id) = 1: agreeing duplicates merge, conflicted persons drop out of targets and pools until the org assignment is fixed. - contracts caught up with the runtime: metrics DESIGN (ids are person UUIDs; person_id is THE key; entity_id = table email key + wire field name), gold/schema.yml person_id notes (no longer "additive, unread"), request DTO field docs, regenerated analytics OpenAPI. - e2e: the well-formed-request helper sends a valid nobody-UUID instead of an email (which now fails validation before reaching the scenario under test), plus an explicit test pinning the email shape to a loud 400. 501 unit tests, clippy pedantic 0, fmt clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Sergei Mozhaev <mozhaev.dev@gmail.com>
…gate' into feat/metrics-person-id-cutover # Conflicts: # src/backend/services/analytics/src/api/metric_results.rs # src/backend/services/analytics/src/domain/metric_results/validation.rs
…on_id cutover
Closes the two blocking review findings (Codex, round 2):
- peer pool conflict guard ordering: the target-cohort filter ran BEFORE
`HAVING uniqExact(cohort_id) = 1`, hiding a conflicted person's
other-cohort row and waving them into the pool. The guard now aggregates
the COMPLETE membership set in an inner query and the target-cohort
filter applies outside it; a unit test pins the order.
- authorization gate integration: merges fix/2035-metric-results-authz-gate
and rewires its whole stack onto the cutover's person UUIDs —
* identity `POST /v1/visible-persons` takes `person_ids: Vec<Uuid>` and
answers visible UUIDs; the email→person resolution layer and its
collation-mismatch defense are gone (that was the pre-cutover bridge;
visibility itself was always person_id-native), along with
`resolve_person_ids_by_emails` and its live tests;
* the analytics gate forwards the validated `person_ids` verbatim and
compares UUID sets; the email normalizer helper is deleted;
* metric-results authorizes BEFORE any ClickHouse work, service
principals bypass, non-visible ids 403 the whole request.
- e2e: the yaml metric rig stays authored in emails (the readable persona
key) and owns the cutover translation — personas get deterministic
uuid5 person ids (lib.identity_stub.person_id_for), identity_persons is
seeded before the gold build so the dbt resolve macro attributes rows,
requests translate email→UUID and responses back for the expect rules;
the identity stub serves the UUID visible-persons contract; the
visible-persons and metric-results suites assert UUID semantics
including the email-shape 400.
513 + 89 unit tests, clippy pedantic clean on both crates, fmt clean.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Sergei Mozhaev <mozhaev.dev@gmail.com>
…REGATION) Caught by the e2e run, not the unit tests (they assert SQL text, not CH acceptance): aliasing `any(cohort_id) AS cohort_id` shadows the source column, and ClickHouse substitutes the aggregate into the enclosing WHERE — code 184, a 500 on every peer view. The alias is now `resolved_cohort_id`, re-projected as `cohort_id` one level up; verified against the live e2e ClickHouse (peer stats return for a translated persona) and pinned in the ordering unit test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Sergei Mozhaev <mozhaev.dev@gmail.com>
… ids The rig seeded identity_persons only for the personas a case REQUESTS, so cohort members that exist purely as seeded HR data resolved to NULL and dropped out of peer pools — 32 of 36 cases failed on a peer `n` one or two short. Every email the yaml mentions (bronze seeds included) now gets a binding; only the requested ids take part in the wire translation. That is the same failure mode the rework exists to eliminate, in miniature: an unresolved person silently shrinks their whole team's comparison pool. 35/36 cases green. The one remaining failure (ai_assistant_activity) is a PRE-EXISTING break on main — verified by running the case on main itself: silver.class_ai_assistant_usage builds empty there too, before any person_id is involved. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Sergei Mozhaev <mozhaev.dev@gmail.com>
Review follow-up: the ignored live tests still spoke the pre-cutover email contract, so neither one tested what it claimed. The 403 case sent an email and would have died on validation's 400 long before the gate; the not-denied case asserted only `!= 403`, which that same 400 satisfies — a request that never reached the gate would have passed it silently. The loopback identity now serves the `person_ids` contract, both cases use named person UUIDs (VISIBLE_PERSON / HIDDEN_PERSON), and the not-denied case asserts the EXACT 500 from the unreachable ClickHouse: the assertion now fails if the request stops earlier for any reason. Also drops the email wording from the /v1/visible-persons OpenAPI summary and request description, and regenerates the analytics spec. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Sergei Mozhaev <mozhaev.dev@gmail.com>
… nil ids Review follow-ups (round 3): - The PUBLISHED identity contract still described emails, so a client generated from it would have gotten a 400 on every call: openapi.json's VisiblePersons request/response are now uuid-typed arrays, and README, DESIGN and ADR-0015 describe person ids. The docs also referenced `persons_repo::resolve_person_ids_by_emails`, deleted with the email bridge — ADR-0015 now records why it is gone instead of pointing at it. - A nil UUID in entity.ids produced a 500: analytics let it through, identity 400s on it, and the gate maps any identity non-success to a server error. It is now rejected in parse_person_ids as the client error it is. - The wildcard short-circuit's semantics are documented where they live: it echoes the request, so the answer is a subset of the input and never a statement that each id exists in the tenant (the SQL branch cannot say more either — absent-from-visible-set and absent-from-tenant are one answer). - Fixture-rig traps: the metric rig now derives the identity stub's visible set from the yaml it is running (`allow_visible`), so a new persona cannot fail as a phantom 403 against a hand-kept list; and the reverse UUID→email map raises on a collision instead of silently dropping one spelling. - The person-only entity contract is stated where a future non-person type will land: ids parse as person UUIDs for every entity type and the gate has a rule for `person` only (fail-closed otherwise) — a first non-person type must bring both halves. 514 + 89 unit tests, clippy workspace clean, fmt clean. e2e re-verified: metrics 35/36 (the one failure reproduces on main), api 93 passed, identity 133 passed; endpoint-coverage gates pass (analytics 28/28 ops with metric-results' 403 now genuinely observed, identity 19/19). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Sergei Mozhaev <mozhaev.dev@gmail.com>
|
…-id-cutover Signed-off-by: Sergei Mozhaev <mozhaev.dev@gmail.com> # Conflicts: # src/backend/services/analytics/src/domain/metric_results/compiler.rs
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change replaces email-based person identifiers with canonical UUIDs across Identity Resolution, Analytics, metric processing, API contracts, synchronization, and end-to-end fixtures. Analytics checks person visibility before metric execution. ChangesIdentity visibility and resolution
Analytics UUID migration
Synchronization safeguards
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant MetricsClient
participant Analytics
participant IdentityResolution
participant MariaDB
participant ClickHouse
MetricsClient->>Analytics: Submit metric request with person UUIDs
Analytics->>IdentityResolution: Check caller visibility
IdentityResolution->>MariaDB: Resolve visible person set
MariaDB-->>IdentityResolution: Return visible UUIDs
IdentityResolution-->>Analytics: Return visible UUID subset
Analytics->>ClickHouse: Execute UUID-filtered metric query
ClickHouse-->>Analytics: Return metric rows
Analytics-->>MetricsClient: Return metric results
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 (1)
src/ingestion/gold/schema.yml (1)
610-617: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the adjacent model description, which still claims the peer view relies on email-grain uniqueness.
This new
person_idtext is accurate. The model description above it (lines 588-593) is now stale. It says uniqueness is per(tenant, entity_type, entity_id, cohort_key)and is "relied on by the analytics service peer view (join fan-out would corrupt percentiles)".The peer view no longer depends on that.
compile_peer_batch_querycollapses both cohort CTEs to person grain withGROUP BY person_idplusHAVING uniqExact(cohort_id) = 1, precisely because email-grain uniqueness does not imply person-grain uniqueness — two emails for one person yield two rows. Fan-out protection now lives in the query, not in this table's uniqueness.📝 Proposed change
Current cohort membership per entity for peer comparison. One row per (tenant, entity_type, entity_id, cohort_key) — uniqueness is asserted by - tests/gold/assert_metric_entity_cohorts_unique.sql and relied on by the - analytics service peer view (join fan-out would corrupt percentiles). The - column set is a published contract probed by the analytics service schema - validator (`COHORT_COLUMNS`). + tests/gold/assert_metric_entity_cohorts_unique.sql. Uniqueness is per + EMAIL, not per person: since the identity cutover the peer view joins on + person_id and collapses to person grain itself, because two emails can + resolve to one person. The column set is a published contract probed by + the analytics service schema validator (`COHORT_COLUMNS`).🤖 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/ingestion/gold/schema.yml` around lines 610 - 617, Update the adjacent model description for the table’s uniqueness constraint to remove the claim that email-grain uniqueness is relied on by the analytics service peer view. State that peer fan-out protection is enforced by compile_peer_batch_query collapsing cohort CTEs to person grain with GROUP BY person_id and HAVING uniqExact(cohort_id) = 1, while preserving the remaining schema semantics.
🧹 Nitpick comments (12)
src/ingestion/tests/e2e/api/test_metric_results.py (1)
45-52: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider adding a nil-UUID 400 test.
test_metric_results_400_non_uuid_person_idscovers the pre-cutover email shape, but a nil UUID (00000000-0000-0000-0000-000000000000) parses as a syntactically valid UUID, so this test does not exercise nil-UUID rejection. The PR commit history states nil UUIDs must return 400 as a distinct case. Add a sibling test asserting a nil UUID inentity.idsreturns 400.✅ Proposed additional test
def test_metric_results_400_nil_uuid_person_id(api) -> None: """A nil UUID is syntactically valid but never a real person; reject it as loudly as a non-UUID string.""" body = _request( metrics=[{"metric_key": "git.commits", "views": [{"view": "period"}]}], entity_ids=("00000000-0000-0000-0000-000000000000",), ) r = api.post("/v1/metric-results", json=body) assert r.status_code == 400, f"status={r.status_code} body={r.text}"🤖 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/ingestion/tests/e2e/api/test_metric_results.py` around lines 45 - 52, Add a sibling test next to test_metric_results_400_non_uuid_person_ids named test_metric_results_400_nil_uuid_person_id, using the nil UUID in entity_ids and the same metric request and API assertion, to verify it returns HTTP 400.src/backend/services/identity-resolution/src/api/visible_persons.rs (1)
117-117: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
#[expect]with a reason instead of a bare#[allow].
#[allow(clippy::expect_used)]has no justification and uses#[allow]where the guideline prefers#[expect]. Add areasonand switch to#[expect]so the lint fails loudly if it stops firing.As per coding guidelines, "Prefer
#[expect(clippy::...)]over#[allow]; provide the justification on the same line as either lint attribute."🔧 Proposed fix
#[cfg(test)] -#[allow(clippy::expect_used)] +#[expect(clippy::expect_used, reason = "test assertions use expect for clear failure messages")] mod tests {🤖 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/visible_persons.rs` at line 117, Replace the bare #[allow(clippy::expect_used)] attribute with #[expect(clippy::expect_used, reason = "...")] at the affected location, providing a concise justification on the same line while preserving the existing lint suppression.Source: Coding guidelines
src/backend/services/identity-resolution/src/infra/db/subchart_repo.rs (1)
125-165: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace rustdoc-style comments with plain comments in this service crate. Four new sites use
///or//!doc comments insideidentity-resolution, which its own README describes as a service, not a shared library crate. The coding guideline bans documentation comments in services regardless of export status or content quality; the fix at each site is the same conversion to//.
src/backend/services/identity-resolution/src/infra/db/subchart_repo.rs#L125-L165: convert the///doc comments (including the "# Errors" sections) onhas_wildcard_grantandvisible_targetsto plain//comments.src/backend/services/identity-resolution/src/api/visible_persons.rs#L22-L24: convert the///doc comment onVisiblePersonsRequestto a plain//comment; also drop the "the earlier email-based draft of this endpoint never shipped" phase note per the "no phase/scope notes in source comments" rule.src/backend/services/identity-resolution/src/infra/db/visible_set_live_tests.rs#L1-L4: convert the//!module doc comment to plain//lines; theINVARIANT:tags themselves are fine, only the//!syntax needs to change.As per coding guidelines, "Use
///documentation comments only on exported items in shared library crates ... Do not add documentation comments to binaries or services, and do not enforcemissing_docs," and "Do not use module headers, issue numbers, or phase/scope notes in source comments."🤖 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/subchart_repo.rs` around lines 125 - 165, Replace Rustdoc comments with plain comments throughout the identity-resolution service. In src/backend/services/identity-resolution/src/infra/db/subchart_repo.rs lines 125-165, convert comments for has_wildcard_grant and visible_targets, including Errors sections, from /// to //. In src/backend/services/identity-resolution/src/api/visible_persons.rs lines 22-24, convert the VisiblePersonsRequest comment to // and remove the email-draft phase note. In src/backend/services/identity-resolution/src/infra/db/visible_set_live_tests.rs lines 1-4, convert the //! module header to // while preserving the INVARIANT tags.Source: Coding guidelines
src/backend/services/analytics/src/api/metric_results.rs (2)
37-53: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftMove metric execution from
query_metric_resultsinto the domain layer.
query_metric_resultsperforms query planning, concurrent execution, result assembly, and response mapping. The added visibility gate extends a handler that is already far beyond the API orchestration limit.Keep this handler to extract → validate → domain call → map → respond. Move the execution workflow into a domain service or free function.
As per coding guidelines, API handlers must use “extract → validate → domain call → map → respond” and stay approximately 30 lines.
🤖 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/analytics/src/api/metric_results.rs` around lines 37 - 53, Refactor query_metric_results so it only extracts request/context data, validates the request, invokes a domain-level metric-results operation, maps its result into MetricResultsResponse, and responds. Move query planning, concurrent execution, result assembly, and related visibility-aware workflow into a domain service or free function, preserving the existing authorization behavior and response contract while keeping the handler near the extract → validate → domain call → map → respond structure.Source: Coding guidelines
42-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove source documentation that duplicates executable contract rules.
Use named tests for non-obvious behavior. Keep published API documentation in OpenAPI and design documents.
src/backend/services/analytics/src/api/metric_results.rs#L42-L44: express visibility-before-query behavior in a named test.src/backend/services/analytics/src/api/metric_results.rs#L122-L124: express canonical UUID response behavior in a named test.src/backend/services/analytics/src/api/http_live_tests.rs#L109-L118: use helper and test names instead of documentation comments.src/backend/services/analytics/src/api/http_live_tests.rs#L135-L136: use the helper name to describe loopback visibility behavior.src/backend/services/analytics/src/api/http_live_tests.rs#L211-L212: keep Authorization forwarding in test setup without explanatory comments.src/backend/services/analytics/src/api/http_live_tests.rs#L284-L288: rename the test to state the downstream-failure rule.src/backend/services/analytics/src/domain/metric_results/dto.rs#L17-L18: document the request contract in OpenAPI.src/backend/services/analytics/src/domain/metric_results/dto.rs#L117-L118: document the response contract in OpenAPI.src/backend/services/analytics/src/domain/metric_results/builder.rs#L56-L58: retain a named UUID wire-contract test.src/backend/services/analytics/src/infra/identity/mod.rs#L43-L55: retain malformed-response coverage in tests.src/backend/services/analytics/src/infra/identity/mod.rs#L150-L153: keep constructor error behavior in tests or API documentation.🤖 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/analytics/src/api/metric_results.rs` around lines 42 - 44, Remove source comments that duplicate executable contracts and replace each behavior description with named tests or helpers: in src/backend/services/analytics/src/api/metric_results.rs:42-44 and :122-124, add or retain named tests for visibility-before-query and canonical UUID responses; in src/backend/services/analytics/src/api/http_live_tests.rs:109-118, :135-136, and :211-212, rely on descriptive helper/test names and keep Authorization forwarding without explanatory comments; rename the downstream-failure test at :284-288. Document request and response contracts in OpenAPI for src/backend/services/analytics/src/domain/metric_results/dto.rs:17-18 and :117-118, retain a named UUID wire-contract test in builder.rs:56-58, malformed-response coverage in infra/identity/mod.rs:43-55, and constructor error behavior in infra/identity/mod.rs:150-153 through tests or API documentation.Source: Coding guidelines
src/backend/services/analytics/src/infra/identity/mod.rs (1)
218-242: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftReturn a typed identity-client error.
visible_person_idsexposesanyhow::Resultfrom reusable infrastructure code. Define athiserrorerror type for transport, HTTP-status, and response-decoding failures, then map it at the API boundary.As per coding guidelines, “Use typed
thiserrorerrors in domain and library code; restrictanyhowto binary entry points and startup wiring.”🤖 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/analytics/src/infra/identity/mod.rs` around lines 218 - 242, Define a custom error type using thiserror that represents the possible failures for the identity client (transport, HTTP status, response decoding), then update the visible_person_ids method to return this typed error instead of anyhow::Result. Map the error sources at the function boundaries: the .send().await? network call, the status check failure, and the .json().await? JSON parsing, ensuring each maps to the appropriate error variant so callers receive structured error information instead of opaque anyhow errors.Source: Coding guidelines
src/backend/services/analytics/src/domain/person_visibility.rs (3)
115-116: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
#[expect]with a justification for the lint suppression.The repository guidelines require
#[expect(clippy::...)]over#[allow], and a justification on the attribute line.♻️ Proposed change
#[cfg(test)] -#[allow(clippy::unwrap_used, clippy::expect_used)] +#[expect(clippy::unwrap_used, clippy::expect_used, reason = "test setup fails loudly")] mod tests {As per coding guidelines: "Prefer
#[expect(clippy::...)]over#[allow]; provide the justification on the same line as either lint attribute."🤖 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/analytics/src/domain/person_visibility.rs` around lines 115 - 116, Replace the #[allow] lint suppression associated with the test module with #[expect], preserving both clippy lints and adding an inline justification on the same attribute line. Keep the existing #[cfg(test)] behavior unchanged.Source: Coding guidelines
62-73: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUnwrap
authorizationat the guard to make the invariant explicit.The code checks
authorization.is_none()and then still forwards anOption<&str>tovisible_person_ids. Alet ... elsebinding removes the second, now-impossibleNonecase from the call site.♻️ Proposed refactor
- if authorization.is_none() { + let Some(authorization) = authorization else { tracing::error!(caller = %caller, "no Authorization header to forward to identity"); return Err(unavailable()); - } + }; let visible = identity - .visible_person_ids(person_ids, authorization) + .visible_person_ids(person_ids, Some(authorization)) .awaitThis assumes
visible_person_idskeeps itsOption<&str>parameter. Confirm the signature insrc/backend/services/analytics/src/infra/identity/mod.rsbefore applying.🤖 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/analytics/src/domain/person_visibility.rs` around lines 62 - 73, In the authorization guard surrounding visible_person_ids, bind the present authorization value with a let-else pattern that returns unavailable() when absent, then pass the unwrapped value to visible_person_ids while preserving its Option<&str> parameter contract.
75-89: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueCount the unmatched ids instead of collecting them.
Both consumers only read
is_empty()andlen(). TheVec<Uuid>allocation is unused.MAX_PERSON_IDSis 1000, so the cost is small, but the count expresses the intent directly.♻️ Proposed refactor
- let unmatched = unmatched_ids(&visible, person_ids); - if unmatched.is_empty() { + let unmatched = unmatched_count(&visible, person_ids); + if unmatched == 0 { return Ok(()); } - Err(denied(caller, unmatched.len())) + Err(denied(caller, unmatched)) } -fn unmatched_ids(visible: &HashSet<Uuid>, person_ids: &[Uuid]) -> Vec<Uuid> { +fn unmatched_count(visible: &HashSet<Uuid>, person_ids: &[Uuid]) -> usize { person_ids .iter() .filter(|person_id| !visible.contains(*person_id)) - .copied() - .collect() + .count() }🤖 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/analytics/src/domain/person_visibility.rs` around lines 75 - 89, Update unmatched_ids to return a count rather than allocating and collecting a Vec<Uuid>, using the existing visibility membership predicate to count person_ids not present in visible. Adjust the caller to compare the count directly with zero and pass it to denied, removing the is_empty and len calls while preserving current behavior.src/backend/services/analytics/src/domain/metric_results/compiler.rs (1)
102-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the person-id binding pair into one helper.
The two-line pattern
params.extend(req.person_ids.iter().map(Uuid::to_string)); let person_id_params = placeholders(req.person_ids.len());repeats in
compile_period_batch_query,compile_timeseries_query,compile_group_ranking_query,compile_capped_timeseries_query,compile_breakdown_query,compile_histogram_query, andcompile_peer_batch_query. Correctness depends on the two lines staying adjacent and in order; a helper makes that structural instead of conventional.♻️ Proposed helper
fn bind_person_ids(params: &mut Vec<String>, person_ids: &[Uuid]) -> String { params.extend(person_ids.iter().map(Uuid::to_string)); placeholders(person_ids.len()) }Then each call site becomes:
- params.extend(req.person_ids.iter().map(Uuid::to_string)); - let person_id_params = placeholders(req.person_ids.len()); + let person_id_params = bind_person_ids(&mut params, &req.person_ids);As per coding guidelines: "Extract repetition into named helpers, and centralize error construction in one helper per failure kind."
🤖 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/analytics/src/domain/metric_results/compiler.rs` around lines 102 - 103, Extract the repeated person-ID binding logic into a bind_person_ids helper that extends params and returns the matching placeholders. Replace the adjacent binding and placeholder statements in compile_period_batch_query, compile_timeseries_query, compile_group_ranking_query, compile_capped_timeseries_query, compile_breakdown_query, compile_histogram_query, and compile_peer_batch_query with calls to this helper, preserving their existing query behavior.Source: Coding guidelines
src/backend/services/analytics/src/domain/metric_results/validation.rs (1)
510-521: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePin the non-person contract with a test instead of prose.
The "DELIBERATE CONTRACT" note claims that ids are parsed as person UUIDs for every entity type, and that a non-person type is a fail-closed 500. The second half is pinned by
entity_type_without_an_authorization_rule_is_not_a_denialinsrc/backend/services/analytics/src/domain/person_visibility.rs. The first half is asserted only in this comment. A test whose name states the rule keeps it true after future edits, and lets the comment shrink.♻️ Proposed test
#[test] fn every_entity_type_parses_ids_as_person_uuids() { // Shape does not vary by entity type; a first non-person type must add // its own id shape here and its own gate rule. let req = shape_request( vec!["a@x.io"], "2026-01-01", "2026-01-31", vec!["ai.x"], ); assert!( validate_request_shape(&req).is_err(), "should reject: non-UUID ids for any entity type" ); }As per coding guidelines: "For non-obvious semantics, add a test whose name states the rule rather than adding a comment."
🤖 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/analytics/src/domain/metric_results/validation.rs` around lines 510 - 521, Replace the prose-only contract in parse_person_ids with a test named every_entity_type_parses_ids_as_person_uuids, using the existing shape_request and validate_request_shape helpers to submit a non-UUID id for an entity type and assert validation fails. Keep the test focused on proving all entity types currently use person UUID parsing, and shrink the surrounding comment accordingly.Source: Coding guidelines
docs/components/backend/analytics/openapi.json (1)
3896-3903: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winDeclare the
entity.idsconstraints indto.rs.
MetricResultsEntity.idsandMetricResultsEntityDto.idsstill useVec<String>insrc/backend/services/analytics/src/domain/metric_results/dto.rs, soopenapi.jsonemitsitems: {type: string}. The validator parses each id as a non-nilUuidand rejects non-UUID values with 400; make the generated schema machine-readable by addingformat = "uuid"to both annotations and a cap matchingMAX_PERSON_IDS.🤖 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 `@docs/components/backend/analytics/openapi.json` around lines 3896 - 3903, In src/backend/services/analytics/src/domain/metric_results/dto.rs, add schema format and length constraints to the ids field in both MetricResultsEntity and MetricResultsEntityDto. Update the Vec<String> annotations for the ids field to declare format="uuid" for each item and add a max_items constraint matching the MAX_PERSON_IDS constant, so the generated OpenAPI schema accurately reflects the validator's requirement that each id is a UUID and the total count is bounded.
🤖 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 `@docs/components/backend/analytics/DESIGN.md`:
- Around line 136-140: The endpoints POST /v1/metrics/{id}/query and POST
/v1/metrics/queries allow caller-supplied person_id and org_unit_id filter
values without visibility validation, enabling users to query data outside their
permitted org scope. Before executing queries that include these filter fields
in these endpoints, validate the requested person_id and org_unit_id values
against the caller's AccessScope using the same authorization pattern as POST
/v1/metric-results (resolving the caller from the gateway JWT and batch-checking
visibility), rejecting the entire request with 403 if any requested id falls
outside the permitted set rather than returning a partial response.
In `@docs/components/backend/identity-resolution/openapi.json`:
- Around line 126-140: Update the VisiblePersonsCommandModel.person_ids schema
to declare the server-enforced array bounds: set maxItems to 1000 and minItems
to 1, while preserving its existing UUID string item definition and required
property.
In `@docs/domain/metrics/specs/DESIGN.md`:
- Around line 450-453: Update the entity-id contract in DESIGN.md to state that
Uuid::nil() is rejected as a client error, alongside the existing UUID parsing
and canonicalization rules. Replace the stale Authorization section with the
authorize_entity_ids behavior: validate requested person UUIDs against the
caller’s visible set before any ClickHouse work, allow service principals to
bypass the check, and reject the entire request with 403 when any requested ID
is not visible.
In `@src/backend/services/analytics/src/domain/metric_results/validation.rs`:
- Around line 199-206: Check req.entity.ids.len() against MAX_PERSON_IDS before
calling parse_person_ids, returning the existing invalid("entity.ids", ...)
response when the submitted count exceeds the cap. Then parse the ids without
the later parsed-count check, so blank entries cannot bypass the cap and
unbounded UUID parsing is avoided.
In `@src/backend/services/identity-resolution/src/api/visible_persons.rs`:
- Around line 37-77: Move the wildcard/visibility branching out of
filter_visible_persons into a pure domain function that accepts is_wildcard,
visible: &HashSet<Uuid>, and requested: Vec<Uuid>, returning the appropriate
echoed or intersected IDs. Update the handler to perform only caller/request
extraction, validation, repository calls, the domain invocation, and response
mapping, using the domain function rather than filtering inline.
In `@src/ingestion/tests/e2e/identity/test_visible_persons.py`:
- Around line 48-51: Add the standard status-code assertion to
test_a_person_in_another_tenant_is_never_visible by storing _check(api,
[seed.EVE]) in a response variable, asserting response.status_code == 200 with
the file’s established diagnostic message pattern, then inspecting
response.json()["visible"] as before.
In `@src/ingestion/tests/e2e/metrics/test_fixtures.py`:
- Around line 65-103: The _seed_identity_persons function must seed a
worker-scoped identity database rather than the shared literal identity
database. Use worker_ctx to derive the database name and apply it consistently
to ensure_database, CREATE/TRUNCATE, and INSERT operations, while preserving the
existing empty-email early return and person-resolution data.
---
Outside diff comments:
In `@src/ingestion/gold/schema.yml`:
- Around line 610-617: Update the adjacent model description for the table’s
uniqueness constraint to remove the claim that email-grain uniqueness is relied
on by the analytics service peer view. State that peer fan-out protection is
enforced by compile_peer_batch_query collapsing cohort CTEs to person grain with
GROUP BY person_id and HAVING uniqExact(cohort_id) = 1, while preserving the
remaining schema semantics.
---
Nitpick comments:
In `@docs/components/backend/analytics/openapi.json`:
- Around line 3896-3903: In
src/backend/services/analytics/src/domain/metric_results/dto.rs, add schema
format and length constraints to the ids field in both MetricResultsEntity and
MetricResultsEntityDto. Update the Vec<String> annotations for the ids field to
declare format="uuid" for each item and add a max_items constraint matching the
MAX_PERSON_IDS constant, so the generated OpenAPI schema accurately reflects the
validator's requirement that each id is a UUID and the total count is bounded.
In `@src/backend/services/analytics/src/api/metric_results.rs`:
- Around line 37-53: Refactor query_metric_results so it only extracts
request/context data, validates the request, invokes a domain-level
metric-results operation, maps its result into MetricResultsResponse, and
responds. Move query planning, concurrent execution, result assembly, and
related visibility-aware workflow into a domain service or free function,
preserving the existing authorization behavior and response contract while
keeping the handler near the extract → validate → domain call → map → respond
structure.
- Around line 42-44: Remove source comments that duplicate executable contracts
and replace each behavior description with named tests or helpers: in
src/backend/services/analytics/src/api/metric_results.rs:42-44 and :122-124, add
or retain named tests for visibility-before-query and canonical UUID responses;
in src/backend/services/analytics/src/api/http_live_tests.rs:109-118, :135-136,
and :211-212, rely on descriptive helper/test names and keep Authorization
forwarding without explanatory comments; rename the downstream-failure test at
:284-288. Document request and response contracts in OpenAPI for
src/backend/services/analytics/src/domain/metric_results/dto.rs:17-18 and
:117-118, retain a named UUID wire-contract test in builder.rs:56-58,
malformed-response coverage in infra/identity/mod.rs:43-55, and constructor
error behavior in infra/identity/mod.rs:150-153 through tests or API
documentation.
In `@src/backend/services/analytics/src/domain/metric_results/compiler.rs`:
- Around line 102-103: Extract the repeated person-ID binding logic into a
bind_person_ids helper that extends params and returns the matching
placeholders. Replace the adjacent binding and placeholder statements in
compile_period_batch_query, compile_timeseries_query,
compile_group_ranking_query, compile_capped_timeseries_query,
compile_breakdown_query, compile_histogram_query, and compile_peer_batch_query
with calls to this helper, preserving their existing query behavior.
In `@src/backend/services/analytics/src/domain/metric_results/validation.rs`:
- Around line 510-521: Replace the prose-only contract in parse_person_ids with
a test named every_entity_type_parses_ids_as_person_uuids, using the existing
shape_request and validate_request_shape helpers to submit a non-UUID id for an
entity type and assert validation fails. Keep the test focused on proving all
entity types currently use person UUID parsing, and shrink the surrounding
comment accordingly.
In `@src/backend/services/analytics/src/domain/person_visibility.rs`:
- Around line 115-116: Replace the #[allow] lint suppression associated with the
test module with #[expect], preserving both clippy lints and adding an inline
justification on the same attribute line. Keep the existing #[cfg(test)]
behavior unchanged.
- Around line 62-73: In the authorization guard surrounding visible_person_ids,
bind the present authorization value with a let-else pattern that returns
unavailable() when absent, then pass the unwrapped value to visible_person_ids
while preserving its Option<&str> parameter contract.
- Around line 75-89: Update unmatched_ids to return a count rather than
allocating and collecting a Vec<Uuid>, using the existing visibility membership
predicate to count person_ids not present in visible. Adjust the caller to
compare the count directly with zero and pass it to denied, removing the
is_empty and len calls while preserving current behavior.
In `@src/backend/services/analytics/src/infra/identity/mod.rs`:
- Around line 218-242: Define a custom error type using thiserror that
represents the possible failures for the identity client (transport, HTTP
status, response decoding), then update the visible_person_ids method to return
this typed error instead of anyhow::Result. Map the error sources at the
function boundaries: the .send().await? network call, the status check failure,
and the .json().await? JSON parsing, ensuring each maps to the appropriate error
variant so callers receive structured error information instead of opaque anyhow
errors.
In `@src/backend/services/identity-resolution/src/api/visible_persons.rs`:
- Line 117: Replace the bare #[allow(clippy::expect_used)] attribute with
#[expect(clippy::expect_used, reason = "...")] at the affected location,
providing a concise justification on the same line while preserving the existing
lint suppression.
In `@src/backend/services/identity-resolution/src/infra/db/subchart_repo.rs`:
- Around line 125-165: Replace Rustdoc comments with plain comments throughout
the identity-resolution service. In
src/backend/services/identity-resolution/src/infra/db/subchart_repo.rs lines
125-165, convert comments for has_wildcard_grant and visible_targets, including
Errors sections, from /// to //. In
src/backend/services/identity-resolution/src/api/visible_persons.rs lines 22-24,
convert the VisiblePersonsRequest comment to // and remove the email-draft phase
note. In
src/backend/services/identity-resolution/src/infra/db/visible_set_live_tests.rs
lines 1-4, convert the //! module header to // while preserving the INVARIANT
tags.
In `@src/ingestion/tests/e2e/api/test_metric_results.py`:
- Around line 45-52: Add a sibling test next to
test_metric_results_400_non_uuid_person_ids named
test_metric_results_400_nil_uuid_person_id, using the nil UUID in entity_ids and
the same metric request and API assertion, to verify it returns HTTP 400.
🪄 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: 1b5d791c-0640-4b74-9a93-05e461453d47
📒 Files selected for processing (32)
docs/components/backend/analytics/DESIGN.mddocs/components/backend/analytics/openapi.jsondocs/components/backend/identity-resolution/identity/README.mddocs/components/backend/identity-resolution/identity/specs/ADR/0015-self-scoped-visibility-read-without-admin.mddocs/components/backend/identity-resolution/identity/specs/DESIGN.mddocs/components/backend/identity-resolution/openapi.jsondocs/domain/metrics/specs/DESIGN.mdsrc/backend/services/analytics/src/api/handlers.rssrc/backend/services/analytics/src/api/http_live_tests.rssrc/backend/services/analytics/src/api/metric_results.rssrc/backend/services/analytics/src/api/mod.rssrc/backend/services/analytics/src/domain/metric_definitions/validator.rssrc/backend/services/analytics/src/domain/metric_results/batch.rssrc/backend/services/analytics/src/domain/metric_results/builder.rssrc/backend/services/analytics/src/domain/metric_results/compiler.rssrc/backend/services/analytics/src/domain/metric_results/dto.rssrc/backend/services/analytics/src/domain/metric_results/validation.rssrc/backend/services/analytics/src/domain/mod.rssrc/backend/services/analytics/src/domain/person_visibility.rssrc/backend/services/analytics/src/gear.rssrc/backend/services/analytics/src/infra/identity/mod.rssrc/backend/services/identity-resolution/src/api/mod.rssrc/backend/services/identity-resolution/src/api/visible_persons.rssrc/backend/services/identity-resolution/src/infra/db/mod.rssrc/backend/services/identity-resolution/src/infra/db/subchart_repo.rssrc/backend/services/identity-resolution/src/infra/db/visible_set_live_tests.rssrc/ingestion/gold/schema.ymlsrc/ingestion/tests/e2e/api/test_metric_results.pysrc/ingestion/tests/e2e/identity/test_visible_persons.pysrc/ingestion/tests/e2e/lib/api_coverage.pysrc/ingestion/tests/e2e/lib/identity_stub.pysrc/ingestion/tests/e2e/metrics/test_fixtures.py
| Every ClickHouse query includes `insight_tenant_id`. User-supplied OData `$filter` values are ANDed with security filters, so users can narrow their view but never widen it. | ||
|
|
||
| **IDOR prevention**: When the frontend includes `org_unit_id eq 'uuid'` in `$filter`, the query engine validates that the requested org unit is within the user's AccessScope before executing the query. Accepting a client-supplied UUID without authorization check would allow any user within a tenant to query any team's data by guessing or enumerating UUIDs. | ||
| **IDOR prevention on person entities**: `POST /v1/metric-results` resolves the caller from the gateway JWT and asks identity, in one batch call, which of the requested person ids that caller may see (`POST /v1/visible-persons`). Any requested id outside the answer refuses the whole request with 403 — never a partial response, which would be indistinguishable from absent data. Reaching identity is required: an unconfigured or unreachable identity service is a server error, so an authorization backend that is down cannot read as "permitted". | ||
|
|
||
| **Planned — IDOR prevention on org units**: validating `org_unit_id` from `$filter` against the caller's AccessScope is *not* implemented. The `POST /v1/metrics/{id}/query` and `POST /v1/metrics/queries` paths interpolate client-supplied `person_id` / `org_unit_id` filter values with no visibility check, so they remain reachable for any id in the tenant. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
Enforce visibility on the legacy metric query endpoints.
POST /v1/metrics/{id}/query and POST /v1/metrics/queries still allow caller-supplied person_id and org_unit_id filters without a visibility check. A tenant user can query data outside their permitted org scope through these routes.
Apply the same authorization rule before query execution, or reject these filter fields until the authorization gate exists.
🤖 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 `@docs/components/backend/analytics/DESIGN.md` around lines 136 - 140, The
endpoints POST /v1/metrics/{id}/query and POST /v1/metrics/queries allow
caller-supplied person_id and org_unit_id filter values without visibility
validation, enabling users to query data outside their permitted org scope.
Before executing queries that include these filter fields in these endpoints,
validate the requested person_id and org_unit_id values against the caller's
AccessScope using the same authorization pattern as POST /v1/metric-results
(resolving the caller from the gateway JWT and batch-checking visibility),
rejecting the entire request with 403 if any requested id falls outside the
permitted set rather than returning a partial response.
| pub async fn filter_visible_persons( | ||
| Extension(state): Extension<Arc<AppState>>, | ||
| Extension(ctx): Extension<SecurityContext>, | ||
| CanonicalJson(req): CanonicalJson<VisiblePersonsRequest>, | ||
| ) -> Result<impl IntoResponse, CanonicalError> { | ||
| let caller = require_caller(&ctx)?; | ||
| let tenant = ctx.subject_tenant_id(); | ||
|
|
||
| let requested = dedup_person_ids(&req.person_ids)?; | ||
|
|
||
| // INVARIANT: the wildcard short-circuit echoes the request, so the answer | ||
| // is a SUBSET OF THE INPUT — never a statement that each id exists in the | ||
| // tenant. The SQL branch cannot say more either (an id absent from the | ||
| // visible set and an id absent from the tenant are one answer), so callers | ||
| // must not read presence here as existence. | ||
| let visible = if subchart_repo::has_wildcard_grant(&state.db, tenant, caller) | ||
| .await | ||
| .map_err(read_err)? | ||
| { | ||
| requested | ||
| } else { | ||
| let visible = subchart_repo::visible_targets( | ||
| &state.db, | ||
| tenant, | ||
| caller, | ||
| &requested, | ||
| &state.config.org_chart_source_type, | ||
| ) | ||
| .await | ||
| .map_err(read_err)? | ||
| .into_iter() | ||
| .collect::<HashSet<_>>(); | ||
|
|
||
| requested | ||
| .into_iter() | ||
| .filter(|person_id| visible.contains(person_id)) | ||
| .collect() | ||
| }; | ||
|
|
||
| Ok(Json(VisiblePersonsResponse { visible })) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Move the visibility decision into a domain function instead of the API layer.
filter_visible_persons calls subchart_repo::has_wildcard_grant and subchart_repo::visible_targets directly, then branches and filters inline. This skips the domain layer entirely. The service's own design doc states the dependency direction must be api → domain → infra, and the general handler guideline requires an orchestration skeleton of extract → validate → domain call → map → respond, with business logic kept out of the API layer.
Extract the branching/filtering logic (the choice between "echo the request" and "intersect with the fetched visible set") into a pure domain function that takes already-fetched values (is_wildcard: bool, visible: &HashSet<Uuid>, requested: Vec<Uuid>) and returns the filtered Vec<Uuid>. This makes the decision testable without a database and keeps the handler as a thin orchestrator that only wires infra calls to that function.
As per coding guidelines, "Keep API handlers to an orchestration skeleton of extract → validate → domain call → map → respond, with approximately 30 lines maximum; keep business logic and serialization formats out of the API layer," and "Run CPU-heavy or blocking work ... through spawn_blocking" style separation is reinforced by DESIGN.md's own statement that "Dependency direction is strict: api → domain → infra."
🤖 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/visible_persons.rs` around
lines 37 - 77, Move the wildcard/visibility branching out of
filter_visible_persons into a pure domain function that accepts is_wildcard,
visible: &HashSet<Uuid>, and requested: Vec<Uuid>, returning the appropriate
echoed or intersected IDs. Update the handler to perform only caller/request
extraction, validation, repository calls, the domain invocation, and response
mapping, using the domain function rather than filtering inline.
Source: Coding guidelines
| def _seed_identity_persons(cfg: SessionConfig, emails: list[str]) -> None: | ||
| """Replace identity.identity_persons with one email binding per persona. | ||
|
|
||
| Runs BEFORE the gold dbt build so resolve_person_id() attributes every | ||
| observation row; the table exists thanks to the on-run-start hook (and is | ||
| normally fed by the identity-resolution persons-sync — the rig plays that | ||
| role here). | ||
| """ | ||
| clickhouse.ensure_database(cfg, "identity") | ||
| clickhouse.execute( | ||
| cfg, | ||
| """ | ||
| CREATE TABLE IF NOT EXISTS identity.identity_persons ( | ||
| id UInt64, value_type String, | ||
| insight_source_type String, insight_source_id UUID, | ||
| insight_tenant_id UUID, | ||
| value_id Nullable(String), value_full_text Nullable(String), | ||
| value Nullable(String), value_effective Nullable(String), | ||
| person_id UUID, author_person_id UUID, reason Nullable(String), | ||
| created_at DateTime64(6, 'UTC'), _synced_at DateTime64(3, 'UTC') | ||
| ) ENGINE = MergeTree ORDER BY id | ||
| """, | ||
| ) | ||
| clickhouse.execute(cfg, "TRUNCATE TABLE identity.identity_persons") | ||
| if not emails: | ||
| return | ||
| rows = ", ".join( | ||
| f"({index + 1}, 'email', 'e2e-rig', generateUUIDv4(), generateUUIDv4(), " | ||
| f"'{email}', '{email}', toUUID('{person_id_for(email)}'), " | ||
| f"toUUID('00000000-0000-0000-0000-000000000000'), now64(6), now64(3))" | ||
| for index, email in enumerate(emails) | ||
| ) | ||
| clickhouse.execute( | ||
| cfg, | ||
| "INSERT INTO identity.identity_persons " # noqa: S608 — values derive from fixture emails | ||
| "(id, value_type, insight_source_type, insight_source_id, insight_tenant_id," | ||
| " value_id, value_effective, person_id, author_person_id, created_at, _synced_at) " | ||
| "VALUES " + rows, | ||
| ) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether resolve_person_id filters identity_persons by tenant,
# and whether other e2e ClickHouse databases are worker-scoped.
rg -n -C 5 'resolve_person_id' src/ingestion
rg -n -C 5 'insight_tenant_id' src/ingestion
rg -n -C 3 'ensure_database' src/ingestion/tests/e2e/lib
rg -n -C 3 'worker_ctx' src/ingestion/tests/e2e/lib/clickhouse.py src/ingestion/tests/e2e/lib/ch_seeder.py 2>/dev/nullRepository: constructorfabric/insight
Length of output: 50382
🏁 Script executed:
#!/bin/bash
set -u
echo "== target test context =="
sed -n '1,260p' src/ingestion/tests/e2e/metrics/test_fixtures.py | cat -n
echo
echo "== WorkerContext and ClickHouse database helpers =="
for f in $(fd -a 'clickhouse.py|ch_seeder.py|conftest.py' src/ingestion/tests/e2e/lib src/ingestion/tests/e2e/metrics | sort); do
echo "--- $f"
wc -l "$f"
done
echo
echo "== focused symbols =="
rg -n --glob '!src/ingestion/connectors/**' -C 4 'resolve_person_id|identity_persons|insight_tenant_id|insight_source_id|WorkerContext|worker_ctx|ensure_database|create_database|database=' src/ingestion/tests/e2e src/ingestion/*.py src/ingestion/dbt 2>/dev/null | head -n 400Repository: constructorfabric/insight
Length of output: 49613
🏁 Script executed:
#!/bin/bash
set -u
echo "== src/ingestion/tests/e2e/lib/clickhouse.py =="
cat -n src/ingestion/tests/e2e/lib/clickhouse.py
echo
echo "== src/ingestion/tests/e2e/lib/dbt_runner.py =="
sed -n '1,180p' src/ingestion/tests/e2e/lib/dbt_runner.py | cat -n
echo
echo "== src/ingestion/tests/e2e/lib/config.py (config fields if present) =="
if [ -f src/ingestion/tests/e2e/lib/config.py ]; then sed -n '1,220p' src/ingestion/tests/e2e/lib/config.py | cat -n; fi
echo
echo "== identity_persons macro references =="
fd -e sql -e yml -e yaml -e py . src/ingestion/dbt macros | xargs rg -n -C 3 '{{ resolve_person_id|identity_persons|create_identity_persons|resolve_person_id'Repository: constructorfabric/insight
Length of output: 18647
🏁 Script executed:
#!/bin/bash
set -u
echo "== candidate files =="
rg -l -n 'resolve_person_id|create_identity_persons|identity_persons' src/ingestion/dbt src/ingestion/tests/e2e || true
echo
echo "== references with correct regex =="
rg -n -C 5 'resolve_person_id|create_identity_persons|identity_persons' src/ingestion/dbt src/ingestion/tests/e2e || true
echo
echo "== all resolve/identity macros by filename =="
fd -e sql . src/ingestion/dbt/macros src/ingestion/dbt 2>/dev/null | xargs rg -n 'CREATE MACRO|resolve_person_id|identity_persons|sipHash128|insight_tenant_id' || trueRepository: constructorfabric/insight
Length of output: 35719
Tenant/source id choice is fine, but shared identity.identity_persons seeding is still not worker-scoped.
resolve_person_id() only filters on value_type / value_effective, so the generated insight_source_id and insight_tenant_id values do not break resolution. The remaining issue is that the seeding writes to the literal identity database without using worker_ctx, so parallel workers can truncate and insert into the same identity.identity_persons table.
🤖 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/ingestion/tests/e2e/metrics/test_fixtures.py` around lines 65 - 103, The
_seed_identity_persons function must seed a worker-scoped identity database
rather than the shared literal identity database. Use worker_ctx to derive the
database name and apply it consistently to ensure_database, CREATE/TRUNCATE, and
INSERT operations, while preserving the existing empty-email early return and
person-resolution data.
Source: Coding guidelines
The SPA routes and the metrics runtime key on the canonical person UUID since the identity cutover, but `POST /v1/profiles` could only be asked by email or by a source-native account id. Resolving a name from the viewer's org subtree instead would have split one permission into two: the visibility gate admits people reachable by explicit or wildcard GRANT as well, so a person could have metrics and no name. `value_type: "person_id"` closes that: no resolution step (the id already IS the key), only validation — a non-UUID or the nil UUID is a 400, never a silent empty resolution, and the source fields are rejected because a person id is tenant-wide. Existence comes from `persons_repo::person_exists` (the append-only log IS the person registry), so an unobserved id answers 404 like an unknown email and the endpoint cannot be used to probe which ids exist. Visibility is untouched, so a person's name and their metrics now answer to ONE rule. A person with no current email becomes reachable at all — structurally impossible through the email key. The fixture tree grows such a persona to pin it. e2e: profile-by-person-id equals profile-by-email (two spellings of one identity, not two views), the emailless persona resolves, visibility gates both keys identically (404 for alice, 200 for bob via the seeded grant), cross-tenant 404, unknown id 404, non-UUID / nil / source-fields 400. Identity suite 141 passed; workspace tests, clippy pedantic and fmt clean. Published spec (value_type enum) and the DESIGN FR row updated. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Sergei Mozhaev <mozhaev.dev@gmail.com>
|
Paired frontend PR: constructorfabric/insight-front#255 — merge the two together. Merging either alone breaks the dashboards: this PR alone makes the API reject the emails the current SPA sends (400 on a non-UUID The frontend side also exercises the new |
…-id-cutover Signed-off-by: Sergei Mozhaev <mozhaev.dev@gmail.com> # Conflicts: # src/backend/services/analytics/src/api/metric_results.rs # src/backend/services/analytics/src/domain/metric_results/validation.rs # src/backend/services/identity-resolution/src/api/visible_persons.rs # src/backend/services/identity-resolution/src/infra/db/persons_repo.rs
The gate compares against `normalize()` — sorted keys, so the diff of a regenerated doc stays reviewable. This branch had committed a raw emitter dump instead (the workflow's own hint suggests that redirect), which is byte-identical in content but fails the drift check. Written with `openapi_spec.py update`, which is the only form that passes. Signed-off-by: Sergei Mozhaev <mozhaev.dev@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/backend/services/identity-resolution/src/infra/db/persons_repo.rs (1)
179-190: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a typed repository error for
person_exists.
persons_repo.rsreturnsanyhow::Result<bool>, but this is library/repository code. Return athiserrorrepository error type and map the database error at this boundary.🤖 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 179 - 190, The person_exists method currently exposes anyhow::Result instead of the repository’s typed error contract. Change its return type to the existing thiserror repository error type and map the database failure from the SeaORM one(db) call at this boundary, while preserving the boolean found.is_some() result.Source: Coding guidelines
🤖 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 283-323: Move the person existence lookup out of
resolve_person_id_mode and expose it through a typed domain operation that owns
the persons_repo::person_exists call and error mapping. Keep the API handler
limited to extracting and validating the request, invoking that domain
operation, and mapping its result to candidate IDs or an empty response;
preserve the existing tenant/person UUID semantics and API error behavior.
- Around line 279-282: Remove the added documentation comments from the service
modules: delete the private helper comment at
src/backend/services/identity-resolution/src/api/handlers.rs:279-282, remove or
relocate the resolution-outcome comment at
src/backend/services/identity-resolution/src/api/handlers.rs:187-189, move the
person-ID wire-contract text from
src/backend/services/identity-resolution/src/domain/profile.rs:15-17 into
OpenAPI, and delete the repository function comment at
src/backend/services/identity-resolution/src/infra/db/persons_repo.rs:169-178.
Preserve non-obvious behavior through behavior-named tests rather than service
documentation.
In `@src/ingestion/tests/e2e/identity/test_profiles.py`:
- Around line 29-32: Add concrete type annotations to _resolve_person_id,
including the project’s HTTP client type, UUID-or-string person_id input, and
its response type. Annotate the api and bob_api parameters in the affected test
functions with their concrete HTTP client type, ensuring every function
signature is typed and no bare Any is exposed.
In `@src/ingestion/tests/e2e/lib/identity_seed.py`:
- Around line 129-142: Define a concrete row type alias for the eight SQL
parameter elements and replace the unparameterized tuple annotations in
_emailless_observation_rows with that alias, including the rows collection and
function return type; preserve the existing row construction and values.
---
Nitpick comments:
In `@src/backend/services/identity-resolution/src/infra/db/persons_repo.rs`:
- Around line 179-190: The person_exists method currently exposes anyhow::Result
instead of the repository’s typed error contract. Change its return type to the
existing thiserror repository error type and map the database failure from the
SeaORM one(db) call at this boundary, while preserving the boolean
found.is_some() result.
🪄 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: 2f058468-9bd2-428c-afaf-79e8520f519e
📒 Files selected for processing (8)
docs/components/backend/analytics/openapi.jsondocs/components/backend/identity-resolution/identity/specs/DESIGN.mddocs/components/backend/identity-resolution/openapi.jsonsrc/backend/services/identity-resolution/src/api/handlers.rssrc/backend/services/identity-resolution/src/domain/profile.rssrc/backend/services/identity-resolution/src/infra/db/persons_repo.rssrc/ingestion/tests/e2e/identity/test_profiles.pysrc/ingestion/tests/e2e/lib/identity_seed.py
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/components/backend/analytics/openapi.json
| /// The `value_type='person_id'` mode: the canonical person needs no resolution | ||
| /// step, so this only validates the key and confirms the person exists in the | ||
| /// tenant. Visibility still applies downstream, so name resolution and metric | ||
| /// access answer to one rule. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove documentation comments from service modules.
These changes add or expand /// comments in service code. Keep API contract text in OpenAPI. Keep non-obvious behavior in behavior-named tests.
src/backend/services/identity-resolution/src/api/handlers.rs#L279-L282: remove the private helper documentation comment.src/backend/services/identity-resolution/src/api/handlers.rs#L187-L189: remove or relocate the resolution-outcome documentation comment.src/backend/services/identity-resolution/src/domain/profile.rs#L15-L17: move the person-ID wire-contract text to OpenAPI.src/backend/services/identity-resolution/src/infra/db/persons_repo.rs#L169-L178: remove the repository function documentation comment.
As per coding guidelines, “Do not add documentation comments to binaries or services.”
📍 Affects 3 files
src/backend/services/identity-resolution/src/api/handlers.rs#L279-L282(this comment)src/backend/services/identity-resolution/src/api/handlers.rs#L187-L189src/backend/services/identity-resolution/src/domain/profile.rs#L15-L17src/backend/services/identity-resolution/src/infra/db/persons_repo.rs#L169-L178
🤖 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
279 - 282, Remove the added documentation comments from the service modules:
delete the private helper comment at
src/backend/services/identity-resolution/src/api/handlers.rs:279-282, remove or
relocate the resolution-outcome comment at
src/backend/services/identity-resolution/src/api/handlers.rs:187-189, move the
person-ID wire-contract text from
src/backend/services/identity-resolution/src/domain/profile.rs:15-17 into
OpenAPI, and delete the repository function comment at
src/backend/services/identity-resolution/src/infra/db/persons_repo.rs:169-178.
Preserve non-obvious behavior through behavior-named tests rather than service
documentation.
Source: Coding guidelines
| async fn resolve_person_id_mode( | ||
| state: &AppState, | ||
| tenant: Uuid, | ||
| req: &ResolveProfileRequest, | ||
| ) -> Result<Vec<Uuid>, CanonicalError> { | ||
| // Cross-field shape matches email's: a person id is tenant-wide, and | ||
| // source scoping is what selects the 'id' mode instead. | ||
| if req.insight_source_type.is_some() || req.insight_source_id.is_some() { | ||
| return Err(ProfileError::invalid_argument() | ||
| .with_field_violation( | ||
| "insight_source_type", | ||
| "insight_source_type / insight_source_id must be null for value_type='person_id'", | ||
| "INVALID", | ||
| ) | ||
| .create()); | ||
| } | ||
|
|
||
| let person_id = Uuid::parse_str(req.value.trim()) | ||
| .ok() | ||
| .filter(|person_id| !person_id.is_nil()) | ||
| .ok_or_else(|| { | ||
| ProfileError::invalid_argument() | ||
| .with_field_violation( | ||
| "value", | ||
| "value must be a person UUID for value_type='person_id'", | ||
| "INVALID", | ||
| ) | ||
| .create() | ||
| })?; | ||
|
|
||
| // A person exists iff the append-only log holds an observation for it; an | ||
| // unknown id yields no candidate, so the caller answers 404 — the same | ||
| // shape an unknown email takes, and no probe for which ids exist. | ||
| let exists = persons_repo::person_exists(&state.db, tenant, person_id) | ||
| .await | ||
| .map_err(|e| { | ||
| tracing::error!(error = %e, "resolve by person id failed"); | ||
| CanonicalError::internal("profile resolution failed").create() | ||
| })?; | ||
| Ok(if exists { vec![person_id] } else { Vec::new() }) | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift
Move person resolution behind the domain boundary.
resolve_person_id_mode calls persons_repo::person_exists from api. Move existence resolution into a typed domain operation. Keep the API code to request parsing, validation, domain invocation, and response mapping.
As per coding guidelines, API handlers must follow “extract → validate → domain call → map → respond.” The design also specifies api → domain → infra.
🤖 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
283 - 323, Move the person existence lookup out of resolve_person_id_mode and
expose it through a typed domain operation that owns the
persons_repo::person_exists call and error mapping. Keep the API handler limited
to extracting and validating the request, invoking that domain operation, and
mapping its result to candidate IDs or an empty response; preserve the existing
tenant/person UUID semantics and API error behavior.
Source: Coding guidelines
…rsion view The connectors-ddl gate re-dumps the snapshot and fails on any drift. The stamp view arrived with its `;` on the SELECT line, while the dumper puts every terminator on its own line, so the gate reports drift on an otherwise-identical file. This is main's own red check (the same run fails on main); the one-line reformat is exactly what CI's regeneration produces, and it unblocks this PR. Signed-off-by: Sergei Mozhaev <mozhaev.dev@gmail.com>
Note on the red
|
…te's edges Review follow-ups on the person_id cutover. The id cap was measured after parsing, which let a caller pad a request past the documented bound (blanks are skipped and duplicates collapse, so the parsed count is the smaller one) and pay for the parse of every entry first. It now counts submitted ids, before parsing, with tests for both padding shapes. Two rules that were prose are now tests: an entity type with no authorization rule fails closed, and a service principal bypasses the gate for any type. The comment that stated them shrinks to the invariant the tests cannot express. Also: `let Some(authorization) = …` removes an impossible None from the identity call, the unmatched ids are counted rather than collected (only the count ever leaves the function), and both new test modules justify their lint suppression with `#[expect(…, reason = …)]`. Docs corrected where they no longer described the code: the metrics Authorization section still called entity-level scoping deferred and warehouse tenant isolation absent — this PR lands the first and the compiler leads every read with `tenant_id = ?`; the cohort model description still claimed the peer view relies on email-grain uniqueness, which the person-grain collapse replaced. Added the nil-UUID rule, the `person_ids` array bounds in the identity OpenAPI doc, an e2e case for the nil UUID, status assertions on two visibility cases, a concrete row type for the seed helpers, and a note on why the identity database in the metric rig is deliberately not worker-scoped. Signed-off-by: Sergei Mozhaev <mozhaev.dev@gmail.com>
|
@coderabbitai Went through all findings from both reviews. Fixed in Fixed
Not doing, with reasons
|
|
Tip For best results, initiate chat on the files or code changes.
The fixes in I accept the PR-scope decisions for the documentation-comment style, handler layout, explanatory security comments, fixture worker isolation, and error typing. The visibility gap on 🐇 ✅ ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
|
Tip For best results, initiate chat on the files or code changes.
The fixes address the reported issues. The validation bound, authorization tests, OpenAPI constraints, and e2e coverage now match the stated contract. I accept the scope decisions for the deferred refactors and documentation-comment consistency work. The remaining visibility coverage gap is suitable for a separate issue. Would you like me to open that follow-up issue? 🐇 ✅ ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! You are interacting with an AI system. |
`GET /v1/persons/{email}` was the last person-keyed route still speaking
email. After the cutover a person id read off a metric result could not be
handed to it: the path value went to identity as `value_type: "email"` and
came back 404. Two contradicting contracts were published side by side on
one service.
The route is now `GET /v1/persons/{person_id}` and the identity client
sends `value_type: "person_id"`, so one key spans the metrics runtime, the
profile facade and the SPA routes.
A pre-cutover email path — or the nil UUID — is a 400 with a field
violation, not a 404 that would read as "no such person", and the parse
happens before the backend check so that client error does not depend on
whether identity is configured.
The e2e identity stub now resolves both key forms, so the facade's
200/404/400 outcomes are all observed; the endpoint-coverage ledger drops
400 from the boilerplate set because it is reachable now.
Signed-off-by: Sergei Mozhaev <mozhaev.dev@gmail.com>
Codex review — both points addressed[P1] The analytics persons facade was still email-only — fixed in Confirmed and it was the right call: Took the first option — converted the facade rather than removing it:
Removal was the alternative — the new SPA already calls Evidence: api suite 96 passed, 14 xfailed (was 93 + 3 failing on the stale runner image — rebuild needed after the Rust change), endpoint-coverage gate green with only the pre-existing [P2] identity README — fixed. The One related thing that is intentional, so nobody removes it: the identity |
There was a problem hiding this comment.
Keep the canonical ID in entity_id
The metrics contract is explicitly polymorphic: entity_type + entity_id. After this cutover, the canonical person UUID is the entity identity, so it should replace the email value in entity_id.
The current shape instead keeps:
entity_id= source-native emailperson_id= canonical UUID and actual runtime key
This creates two competing identity columns. entity_id no longer identifies the measured entity despite its name and documented role, while the entire runtime is specialized around person_id.
This causes concrete inconsistencies throughout the stack:
-
All five observation tables remain ordered by the obsolete email key:
src/ingestion/gold/ai_metric_observations.sqlsrc/ingestion/gold/collab_metric_observations.sqlsrc/ingestion/gold/git_metric_observations.sqlsrc/ingestion/gold/task_metric_observations.sqlsrc/ingestion/gold/wiki_metric_observations.sql
Their
ORDER BYis still(source_key, measure_key, entity_id, metric_date), while runtime queries filter and group byperson_id. Becauseentity_idsits beforemetric_date, the old email key also prevents effective pruning by the canonical person and subsequent date component. -
The gold models group by both
entity_idandperson_id. Therefore, multiple emails belonging to one canonical person remain separate rows in the serving tables and must be recombined by every runtime query. The gold observation grain remains source-email grain rather than canonical-entity grain. -
metric_entity_cohorts_currentalso remains email-grained. The peer compiler now needs additional grouping and conflict-handling CTEs to collapse those rows back to person grain. This is compensating in the query layer for a non-canonical serving model. -
OBSERVATION_COLUMNSandCOHORT_COLUMNSnow require bothentity_idandperson_id, making the duplicated identity shape part of the permanent runtime contract. -
gold/schema.ymldescribesentity_idas a lowercased email while declaringperson_idto be the actual runtime key. This contradicts the metrics design statement thatentity_type + entity_ididentifies the measured entity. -
Validation, compiler rows, batching, builders, and query results were renamed from
entity_idtoperson_id, only for the builders to translateperson_idback into the wire fieldentity_id. That boundary translation is a symptom that the internal model no longer matches the public entity abstraction. -
Comments in the gold models still state that
entity_idis the runtime key, which is no longer true after this PR.
Source email may still be required for resolution coverage and provenance, but it should be represented explicitly as source_entity_id or retained in an upstream resolution model. identity_resolution_coverage can count unresolved source identifiers from that layer; it does not require the source email to remain the canonical gold entity_id.
The intended serving shape should be:
entity_type = 'person'entity_id = <canonical person UUID>- optional
source_entity_id = <source-native email>in the resolution/provenance layer
The runtime can then continue using entity_id throughout, the observation tables can be materialized at canonical entity grain, the sort key remains aligned with runtime access patterns, and future entity types fit the existing abstraction without another domain-wide rename.
Please implement the final canonical shape in this cutover rather than shipping person_id as a second permanent identity column and adapting every downstream layer around that duplication.
🤖 connectors-ddl snapshot driftThe committed snapshot does not match what this branch actually produces. The regenerated snapshot is waiting in #2141 — review the DDL diff there and merge it into this branch; the gate re-runs on your merge. Refreshed on every drifting gate run (the regen branch is force-pushed), so it reflects this branch as of the last completed run. |
…table bootstrap-db + dump-ddl over the models already on this branch: the five evidence relations gained source_entity_id when resolution moved to build time, and metric_entity_cohorts_current became a MergeTree table — the committed dump still described the pre-resolution shapes. Signed-off-by: Sergei Mozhaev <mozhaev.dev@gmail.com>
The same class of red check as last time — cargo fmt --check caught an unsorted test import, and the pinned ruff (pre-commit's 0.15.21) had four import-order errors plus formatting drift in the e2e files this branch touches. main's copies are clean, so all of it was ours. Signed-off-by: Sergei Mozhaev <mozhaev.dev@gmail.com>
…uses Rebase brings #2098, which changes what every person-keyed route TAKES: the canonical person UUID, not an email. That is not an addition this suite could ignore — it inverts assertions already here. GET /v1/persons/{email} becomes /v1/persons/{person_id} POST /v1/metric-results entity.ids are person UUIDs POST /v1/metric-drilldown entity.id is a person UUID POST /v1/profiles gains value_type="person_id" `test_metric_results_403_by_uuid` is the sharpest case and it is gone. It asserted that asking by canonical UUID was REFUSED — true when the endpoint keyed on email, because a UUID matched nobody in the caller's visible set. The UUID is the right key now, so the same request is a 200 and the test was asserting the old world. Replaced by the refusal that still exists: a person genuinely outside the caller's scope, which reaches the same gate for a reason that will survive the next cutover. New coverage where the cutover created a contract rather than moved one — an email and the nil UUID are both loud 400s on all three routes. The email case is the one that earns its place: it is what these routes took last week, so an unmigrated caller sends one in earnest, and the old failure mode was a 200 with `value: null` that no consumer could tell from a person with no activity. That ambiguity is what the cutover removed; this is what keeps it removed. `/v1/persons/{id}` also joins the non-UUID matrix, which it could not before — it binds Path<Uuid> now. And `/v1/profiles` is asserted to answer the SAME profile through both keys rather than merely 200 through each: a person is one record reachable two ways, not two records that agree today. Generated models regenerated; SOME_EMAIL stays for identity's `/internal/persons/by-email`, a lookup BY email that the cutover left alone. 272 collected becomes 283. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
… it reads The stand came up green and empty. dbt reported PASS=18 and built every gold model; all four observation tables held nothing, and the readiness gate caught it — "not rebuilt since …", which is what an EMPTY table looks like through `system.parts`, because a table with no rows has no parts. The cause is #2098's other half. Gold observation models now LEFT JOIN `dbt/macros/resolve_person_id.sql` over `identity.identity_persons` and DROP every row that does not resolve (`resolved_only()`), because `entity_id` in gold IS the canonical person id. That log is filled by the identity service's persons-sync in a deployment. The stand runs no sync, so nothing filled it, so nothing resolved, so every observation was discarded — silently, with a green dbt run either side of it. Seeded here, for the same reason this seed stands in for the connectors upstream: the stand has no orchestrator to run the real thing. Whole roster, not just the measured people — the admin operator has no activity but still has to RESOLVE, or a request naming them reads as an unknown person rather than a person with nothing. Worth recording what this says about the readiness gate: it was added to stop a stand with a silent generator gap being reported ready, and this is precisely that failure arriving from a direction nobody anticipated — an upstream join condition rather than a broken generator. Without it the suite would have run against empty gold and failed as ~40 unrelated assertions. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
… caught up Two more consequences of #2098, one a test fix and one a finding. **`POST /v1/visible-persons` moved to person ids.** `VisiblePersonsRequest` is `person_ids: Vec<Uuid>` now and the response is `Vec<Uuid>`, so the suite was sending the pre-cutover shape and getting a 400. Updated, along with the schema. The tenant half of that route also got sharper and the test now says so: a wildcard grant used to ECHO the request back, so a wildcard holder in tenant A could have tenant B's ids confirmed as visible. It is intersected with the tenant's persons log now — which is exactly what the other-tenant case here asserts, so the assertion gained meaning without changing shape. **The pinned frontend predates the cutover, and the stand caught it.** `src/frontend/helm/Chart.yaml` pins build 2026.07.31; #2098 landed 2026-08-03. That build still calls person-keyed routes with emails, so the post-cutover backend refuses them: the failed run logged 140 `POST /v1/metric-results` 400s with a HeadlessChrome user-agent. The browser is doing it, not the suite. Marked xfail(strict) rather than softened, because nothing here is wrong. This is the cross-repo integration break a deployed stand exists to find and that neither repo's own tests can see — insight-front tests against its own expectations, and the backend tests its own handlers. Strict so the marker cannot outlive the skew: the first frontend release that speaks person ids makes these XPASS and fails the run, which is the signal to delete it. I could not check whether such a release already exists — listing ghcr tags needs a read:packages scope this token lacks. If insight-front has shipped post-cutover, the fix is a chart bump instead of this marker. Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
…son_id}
The BLOCKED table still keyed the person lookup by `{email}`, which is the
route the identity cutover (#2098) renamed. The gate reported it as stale on
its first run afterwards and was ignored; this is that advisory acted on.
It is not cosmetic. With the key naming a route the spec no longer has, the
403 and 409 that `.standard_errors` stamps on `GET /v1/persons/{person_id}`
were counted as coverable and then reported as gaps — two entries in the
uncovered-code list that no test could ever have closed, and two codes
inflating the denominator against the suite.
Neither is reachable. `handlers::get_person` answers 200, 400 for an id that
is not a non-nil UUID, and 404 when identity resolves nobody; every other
outcome, including a refusal from identity, goes through an error arm that
maps it to `internal`. So a 403 upstream leaves analytics as a 500, and there
is no conflict path at all.
Signed-off-by: Konstantin Tursunov <Konstantin.Tursunov@constructor.tech>
3 participants
What
Iteration 2 of the metrics
person_idrework: the runtime now keys on the canonical person the identity pipeline resolves into the gold tables (delivered by #2065 + #2066). Two people's accounts that resolve to one person finally read as one person; rows identity cannot resolve (person_id IS NULL) drop out of every query — the epic's excluded, not guessed, now enforced by the key itself.Also integrates and re-contracts #2082 (@aleksdotbar's visibility gate). Its commits are merged here, and its whole stack moved from emails to person UUIDs — see the coordination note at the bottom.
How
Validation —
entity.idsparse as UUIDs intoperson_ids: Vec<Uuid>(parse-don't-validate). The pre-cutover email shape and the nil UUID are both loud400s, never silent empty results. The email lowercasing normalizer is gone.Compiler — every view (period / timeseries incl. capped / breakdown / histogram / ranking / peer) filters
person_id IN (…), groups and orders by it, projectstoString(assumeNotNull(person_id)).Peer — the cohort join runs
person_id = person_idinstead of string email equality, the fragile HR-email↔source-email match this rework exists to kill. Two hardenings, both from review:HAVING uniqExact(cohort_id) = 1, evaluated over the person's complete membership set (the target-cohort filter applies outside the guard — filtering first would hide the conflicting row and wave the person through). A person whose emails claim different departments leaves peer comparison until the org assignment is fixed.Authorization — the gate now forwards validated person UUIDs to identity's
POST /v1/visible-persons(which takesperson_idsand answers UUIDs; its email-resolution layer, needed only as the pre-cutover bridge, is deleted along withresolve_person_ids_by_emails). It runs before any ClickHouse work; service principals bypass; a single non-visible id refuses the whole request rather than silently dropping an entity.Schema validator —
person_idjoinsOBSERVATION_COLUMNS/COHORT_COLUMNS, as their comment promised: a table without it would 500 every query, so the probe now marks the source unavailable upfront.Naming discipline — everything carrying the person UUID is named
person_id/person_idsthrough validation, compiler rows, batch demux and builder.entity_idsurvives only as the wire field name, documented as a seam at the single builder/DTO boundary.Contracts — metrics DESIGN,
gold/schema.yml, request DTO docs, and both OpenAPI specs (analytics + identity, the published one described a contract that no longer existed). The person-only entity contract is stated in code where a first non-person type will land: ids parse as person UUIDs for every type and the gate rules onpersonalone (fail-closed), so a new type must bring both halves.Testing
cargo test --workspace: 514 + 89 + … , 0 failed · clippy pedantic workspace-clean · fmt clean.person_visibility.rs100%,compiler.rs97.8%,builder.rs92.9%,batch.rs89.3%,validation.rs87.6%.!= 403— which that 400 satisfies. Both now use named person UUIDs, and the not-denied case asserts the exact 500 from the unreachable ClickHouse, so it fails if the request stops earlier.403is now genuinely observed, not BLOCKED boilerplate), identity 19/19.identity.identity_personsis seeded before the gold build so the dbt resolve macro attributes rows, requests translate email→UUID and responses back. Two latent traps closed: the stub's visible set is derived from the yaml under test (a new persona can't fail as a phantom 403), and a UUID collision in the reverse map raises instead of silently dropping a spelling.Known, not ours:
metrics/ai_assistant_activityfails here and onmainalike (silver.class_ai_assistant_usagebuilds empty, before anyperson_idis involved) — filed separately, not a cutover regression.Coordination — #2082
@aleksdotbar — your visibility gate is merged into this branch (commits preserved), but its contract had to change:
entity.idsis UUIDs now, so/v1/visible-personstakesperson_idsand the email→person resolution step is gone. Visibility itself was alwaysperson_id-native inside identity, so the gate got simpler, not harder. Please look at how it landed and say which you prefer: (a) #2082 merges first as-is and this PR rebases to re-contract it, or (b) this PR supersedes it and #2082 closes with a pointer here.Rollout order
The gold
person_idcolumn is populated by a dbt build that reads a ClickHousemirror of the identity persons log. Deploy in this order:
persons-sync— populatesidentity.identity_persons. The CronJob runsdaily at 06:45, and nothing triggers an initial run, so on a fresh stand
run the Job by hand or every
person_idresolves NULL until the next tick.person_idinto the observation and cohortrelations. Until it runs, the old gold tables lack the column and the
analytics schema validator reports
schema_status: errorfor them.Between steps the failure mode is honest emptiness, not a 500: a metric whose
person_idis NULL contributes no rows.Tenant stamp caveat
Compiled queries filter on the warehouse
tenant_id, bound from the caller'sgateway JWT. The platform has no defined mapping between the control-plane
tenant id and the
tenant_idstring stamped at ingestion. On a stand where thetwo differ, every metric comes back empty with no log line saying why — a
configuration problem that reads exactly like "no data for this period".
Tracked in #2107 (deploy-time warning when the JWT tenant matches no stamp). The
predicate itself is not from this PR; it arrived with the metric-results
authorization work and is inherited here.
Refs #1873; builds on #2065, #2066; integrates #2082.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
POST /v1/visible-personsto return person IDs visible to the authenticated caller.Bug Fixes
Documentation