test(identity): cover the person_id contract at the HTTP layer - #2150
Conversation
📝 WalkthroughWalkthroughThis change adds shared MariaDB fixtures and live HTTP tests for identity-resolution visibility and profile routes. It also adds validation tests and requires ChangesIdentity resolution test coverage
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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 |
The identity half of the person_id cutover shipped with its API layer
effectively untested: `api/handlers.rs` sat at 3.9% line coverage and
`persons_repo::person_exists` had no test at all. The repo-level live
tests reach the SQL, but nothing exercised extractor -> gate -> handler,
so the status codes analytics and the SPA depend on were unpinned.
Add `api/http_live_tests.rs`, driving the real route table through
`tower::oneshot` against a live MariaDB:
- `POST /v1/visible-persons` — the subtree answer, 400 for a request
naming nobody (empty / all-nil / non-UUID / over the cap), 401 without
an identified caller, and the wildcard branch: a wildcard grant covers
everyone IN THE TENANT, so a foreign or invented id must not come back
confirmed. Analytics reads this answer as authorization, so that bound
is now pinned at the layer analytics consumes.
- `POST /v1/profiles` with `value_type='person_id'` — resolution, a
person with no email (the case the key change exists for), 404 outside
the visible set and 200 once granted, 404 cross-tenant, 404 unknown,
400 for an email / the nil UUID / source fields.
The fixture moves to `infra/db/test_fixture.rs` so both suites share one
definition instead of drifting; it gains `in_another_tenant()` and
`emailless_person()`. Two live tests cover `person_exists` (tenant-bound,
and a person with no email still exists).
On the e2e side, `POST /v1/visible-persons` gains the 400 (empty, nil,
over-cap) and 401 cases, and joins `IDENTITY_RUST_REQUIRED_EXTRA` for
`{400, 401}` — the spec declares only 200, so until now the gate asked
nothing of this endpoint beyond "a test called it once".
These tests are deliberately NOT `#[ignore]`d: the identity CI job runs
`cargo test` without `--include-ignored`, so they self-skip on a missing
`INTEGRATION_TESTS_MARIADB_URL` like every other live suite here.
Coverage of the touched files: `api/handlers.rs` 3.9% -> 53.9%,
`api/visible_persons.rs` 58.4% -> 95.5%, `infra/db/persons_repo.rs`
15.5% -> 62.7%; crate total 42.6% -> 55.1%.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Signed-off-by: Sergei Mozhaev <mozhaev.dev@gmail.com>
d58846c to
b097c1c
Compare
…n-id-coverage # Conflicts: # src/ingestion/tests/e2e/identity/test_visible_persons.py
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (8)
src/backend/services/identity-resolution/src/infra/db/visible_set_live_tests.rs (1)
122-122: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd an assertion message.
This
assert!reports only a boolean on failure. Every other assertion in the file names the rule it checks.As per coding guidelines: "use table-driven loops with per-case assertion messages".
♻️ Proposed change
- assert!(persons_repo::person_exists(&f.db, f.tenant, person).await?); + assert!( + persons_repo::person_exists(&f.db, f.tenant, person).await?, + "a person observed without an email still 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 `@src/backend/services/identity-resolution/src/infra/db/visible_set_live_tests.rs` at line 122, Add a descriptive assertion message to the persons_repo::person_exists assertion in the relevant test, identifying the rule or condition being verified and following the file’s existing per-case assertion-message style.Source: Coding guidelines
src/ingestion/tests/e2e/lib/api_coverage.py (1)
120-125: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueShorten the comment to one line.
The guidelines for
src/ingestion/**/*.pylimit comments to one line. This comment runs to five. The key fact is that the spec declares only200, so the gate needs these refusals stated explicitly.The neighbouring entry at lines 115-118 uses the same multi-line style, so treat this as optional and consistent with the file, or shorten both.
As per coding guidelines: "Add comments only when code cannot express the reason ... keep them to one line".
🤖 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/lib/api_coverage.py` around lines 120 - 125, Shorten the explanatory comment above the "POST /v1/visible-persons" entry to a single line stating that the spec declares only 200, so the expected 400 and 401 refusals must be listed explicitly; optionally apply the same one-line style to the neighboring entry for consistency.Source: Coding guidelines
src/backend/services/identity-resolution/src/infra/db/test_fixture.rs (3)
48-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the duplicated
personsINSERT into one helper.
personandemailless_personrepeat the same column list and the same seven bound values. Only thevalue_typeliteral and thevalue_idexpression differ. A shared private helper keeps both call shapes and leaves one copy of the SQL.As per coding guidelines: "Extract repetition into named helpers".
♻️ Proposed refactor
pub(crate) async fn person(&self, email: &str) -> anyhow::Result<Uuid> { - let person_id = Uuid::now_v7(); - self.exec( - "INSERT INTO persons (value_type, insight_source_type, insight_source_id, - insight_tenant_id, value_id, person_id, author_person_id, reason) - VALUES ('email', ?, ?, ?, ?, ?, ?, ?)", - [ - SOURCE_TYPE.into(), - bytes(self.source_id), - bytes(self.tenant), - email.into(), - bytes(person_id), - bytes(person_id), - FIXTURE_REASON.into(), - ], - ) - .await?; - Ok(person_id) + self.insert_person("email", email).await } - /// A person the log knows without an email observation — the shape the - /// `person_id` key exists to serve and the email key structurally cannot. pub(crate) async fn emailless_person(&self) -> anyhow::Result<Uuid> { let person_id = Uuid::now_v7(); - self.exec( - "INSERT INTO persons (value_type, insight_source_type, insight_source_id, - insight_tenant_id, value_id, person_id, author_person_id, reason) - VALUES ('id', ?, ?, ?, ?, ?, ?, ?)", - [ - SOURCE_TYPE.into(), - bytes(self.source_id), - bytes(self.tenant), - format!("acct-{}", person_id.simple()).into(), - bytes(person_id), - bytes(person_id), - FIXTURE_REASON.into(), - ], - ) - .await?; - Ok(person_id) + self.insert_person_with_id("id", &format!("acct-{}", person_id.simple()), person_id) + .await + } + + async fn insert_person(&self, value_type: &str, value_id: &str) -> anyhow::Result<Uuid> { + self.insert_person_with_id(value_type, value_id, Uuid::now_v7()) + .await + } + + async fn insert_person_with_id( + &self, + value_type: &str, + value_id: &str, + person_id: Uuid, + ) -> anyhow::Result<Uuid> { + self.exec( + "INSERT INTO persons (value_type, insight_source_type, insight_source_id, + insight_tenant_id, value_id, person_id, author_person_id, reason) + VALUES (?, ?, ?, ?, ?, ?, ?, ?)", + [ + value_type.into(), + SOURCE_TYPE.into(), + bytes(self.source_id), + bytes(self.tenant), + value_id.into(), + bytes(person_id), + bytes(person_id), + FIXTURE_REASON.into(), + ], + ) + .await?; + Ok(person_id) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/backend/services/identity-resolution/src/infra/db/test_fixture.rs` around lines 48 - 88, Extract the duplicated INSERT logic from person and emailless_person into a private helper that accepts the differing value_type and value_id inputs, while retaining the shared source, tenant, person, author, and reason bindings. Update both methods to call the helper and return the generated person_id, leaving a single copy of the persons INSERT SQL.Source: Coding guidelines
25-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFixture rows are never removed from the shared CI database.
Each call mints a new tenant, so tests stay isolated. No code deletes the rows afterwards. Every CI run therefore adds rows to
persons,org_chart,visibility, andperson_rolesin the shared MariaDB instance, and the tables grow without bound.Add a cleanup path keyed by
reason = FIXTURE_REASONandinsight_tenant_id = self.tenant. A tenant-scoped delete stays safe next to the e2e seeder, which the module header notes deletes by reason with no tenant filter.Do you want me to generate the cleanup helper and wire it into the suites?
🤖 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/test_fixture.rs` around lines 25 - 35, Add a cleanup helper for Fixture that deletes rows from persons, org_chart, visibility, and person_roles using reason = FIXTURE_REASON and insight_tenant_id = self.tenant, then wire it into each test suite so cleanup runs after fixture use. Keep the deletion tenant-scoped and use the existing database connection patterns near fixture_or_skip and connect_single.
1-8: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueModule doc headers in a service. Both new Rust test files open with a multi-line
//!header and add///comments to private items. The coding guidelines forbid module headers in source comments and restrict///to exported items in shared library crates. Both headers also repeat the same#[ignore]and runtime-skip context.
src/backend/services/identity-resolution/src/infra/db/test_fixture.rs#L1-L8: remove the//!header and the///comments at lines 38-39, 68-69, and 108. Encode theFIXTURE_REASONversuse2e-seedrule as a test named after the rule.src/backend/services/identity-resolution/src/api/http_live_tests.rs#L1-L13: remove the//!header and the///comment at line 35. Move theSecurityContextbypass rationale into the PR or a design-document section.State the shared
#[ignore]and CI constraint once, in the CI job configuration or the PR description, rather than in each file.As per coding guidelines: "Do not use module headers, issue numbers, or phase/scope notes in source comments" and "Do not add documentation comments to binaries or services".
🤖 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/test_fixture.rs` around lines 1 - 8, Remove the module-level //! headers from src/backend/services/identity-resolution/src/infra/db/test_fixture.rs lines 1-8 and src/backend/services/identity-resolution/src/api/http_live_tests.rs lines 1-13, and remove the private-item /// comments at the specified locations in test_fixture.rs and line 35 in http_live_tests.rs. Add a test named for the FIXTURE_REASON versus e2e-seed rule, move the SecurityContext bypass rationale to the PR or design documentation, and state the shared #[ignore] and CI runtime-skip constraint once in CI configuration or the PR description.Source: Coding guidelines
src/backend/services/identity-resolution/src/api/http_live_tests.rs (2)
42-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
app_foris an unused seam. Either use it or inline it.Every test calls
app(&f, caller), which always setstenant: f.tenant. No test constructs aCallerwhose tenant differs from the fixture tenant, so theapp_forindirection carries no current caller.The seam does point at a real coverage gap. All tenant-isolation cases here vary the data tenant while the caller tenant stays fixed. A test that drives a caller whose
subject_tenant_idis a foreign tenant would prove the handlers scope reads by the context tenant. Add that case, or inlineapp_forintoapp.As per coding guidelines: "do not add speculative API surface".
🤖 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/http_live_tests.rs` around lines 42 - 63, Remove the unused app_for seam by inlining its setup into app, unless adding a concrete test that constructs a Caller with a tenant different from f.tenant and verifies tenant-scoped handler reads. Do not retain app_for as speculative API surface without such coverage.Source: Coding guidelines
236-251: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe test name does not match what it drives.
inject_host_contextalways inserts a validSecurityContext. This test therefore proves that the gate rejects a nilsubject_id. It does not prove that a request with noSecurityContextextension is rejected, which is the shape the gateway produces for an unidentified caller.Rename this case to state the rule it checks, for example
a_nil_subject_id_is_unauthenticated. Then add a second case that posts to a router built without the injection layer, so the missing-context branch of the extractor is covered in process as well as in the e2e suite.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/identity-resolution/src/api/http_live_tests.rs` around lines 236 - 251, The test name `visible_persons_without_a_caller_is_unauthenticated` incorrectly implies missing security context; rename it to reflect that a nil subject ID is rejected. Add a separate test using a router without `inject_host_context` that posts to the same endpoint and asserts `StatusCode::UNAUTHORIZED`, covering the missing-context extractor branch.Source: Coding guidelines
tests/stand/api/identity/test_visible_persons.py (1)
36-37: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe cap is duplicated across the language boundary.
_MAX_PERSON_IDSrestatesvisible_persons.rs::MAX_PERSON_IDS. Nothing enforces the link. If the Rust cap drops below 1000,test_more_ids_than_the_cap_is_a_400still returns 400 and still passes, but it no longer tests the boundary. The failure is silent.The exact boundary is already asserted against the real constant in
visible_persons.rslines 147-160, so this is a drift risk rather than a coverage hole. Consider exposing the limit through the API or a generated constant if the value is expected to change.🤖 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 `@tests/stand/api/identity/test_visible_persons.py` around lines 36 - 37, The test’s _MAX_PERSON_IDS duplicates the Rust limit without enforcing synchronization, so the boundary test can silently drift. Replace the hardcoded value with the exposed or generated MAX_PERSON_IDS value from visible_persons.rs, or otherwise obtain the limit through the API, and update test_more_ids_than_the_cap_is_a_400 to derive its input from that authoritative value.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/backend/services/identity-resolution/src/api/http_live_tests.rs`:
- Around line 42-63: Remove the unused app_for seam by inlining its setup into
app, unless adding a concrete test that constructs a Caller with a tenant
different from f.tenant and verifies tenant-scoped handler reads. Do not retain
app_for as speculative API surface without such coverage.
- Around line 236-251: The test name
`visible_persons_without_a_caller_is_unauthenticated` incorrectly implies
missing security context; rename it to reflect that a nil subject ID is
rejected. Add a separate test using a router without `inject_host_context` that
posts to the same endpoint and asserts `StatusCode::UNAUTHORIZED`, covering the
missing-context extractor branch.
In `@src/backend/services/identity-resolution/src/infra/db/test_fixture.rs`:
- Around line 48-88: Extract the duplicated INSERT logic from person and
emailless_person into a private helper that accepts the differing value_type and
value_id inputs, while retaining the shared source, tenant, person, author, and
reason bindings. Update both methods to call the helper and return the generated
person_id, leaving a single copy of the persons INSERT SQL.
- Around line 25-35: Add a cleanup helper for Fixture that deletes rows from
persons, org_chart, visibility, and person_roles using reason = FIXTURE_REASON
and insight_tenant_id = self.tenant, then wire it into each test suite so
cleanup runs after fixture use. Keep the deletion tenant-scoped and use the
existing database connection patterns near fixture_or_skip and connect_single.
- Around line 1-8: Remove the module-level //! headers from
src/backend/services/identity-resolution/src/infra/db/test_fixture.rs lines 1-8
and src/backend/services/identity-resolution/src/api/http_live_tests.rs lines
1-13, and remove the private-item /// comments at the specified locations in
test_fixture.rs and line 35 in http_live_tests.rs. Add a test named for the
FIXTURE_REASON versus e2e-seed rule, move the SecurityContext bypass rationale
to the PR or design documentation, and state the shared #[ignore] and CI
runtime-skip constraint once in CI configuration or the PR description.
In
`@src/backend/services/identity-resolution/src/infra/db/visible_set_live_tests.rs`:
- Line 122: Add a descriptive assertion message to the
persons_repo::person_exists assertion in the relevant test, identifying the rule
or condition being verified and following the file’s existing per-case
assertion-message style.
In `@src/ingestion/tests/e2e/lib/api_coverage.py`:
- Around line 120-125: Shorten the explanatory comment above the "POST
/v1/visible-persons" entry to a single line stating that the spec declares only
200, so the expected 400 and 401 refusals must be listed explicitly; optionally
apply the same one-line style to the neighboring entry for consistency.
In `@tests/stand/api/identity/test_visible_persons.py`:
- Around line 36-37: The test’s _MAX_PERSON_IDS duplicates the Rust limit
without enforcing synchronization, so the boundary test can silently drift.
Replace the hardcoded value with the exposed or generated MAX_PERSON_IDS value
from visible_persons.rs, or otherwise obtain the limit through the API, and
update test_more_ids_than_the_cap_is_a_400 to derive its input from that
authoritative value.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: b618a54a-6a79-4237-b5c4-40ecbd39b79f
⛔ Files ignored due to path filters (1)
src/backend/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
src/backend/services/identity-resolution/Cargo.tomlsrc/backend/services/identity-resolution/src/api/http_live_tests.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/test_fixture.rssrc/backend/services/identity-resolution/src/infra/db/visible_set_live_tests.rssrc/ingestion/tests/e2e/lib/api_coverage.pytests/stand/api/identity/test_visible_persons.py
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/backend/services/identity-resolution/src/infra/db/test_fixture.rs (2)
1-8: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace fixture documentation comments with code and named tests.
//!module headers are not allowed in this service.///comments on thesepub(crate)fixture items are also not allowed.Remove the descriptive comments. Keep a short
// INVARIANT:comment only when code cannot express the reason. Add a test with a rule-based name for any remaining non-obvious behavior.As per coding guidelines, use
///only on exported shared-library items and add a named test for non-obvious semantics.Also applies to: 38-39, 68-69, 108-108
🤖 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/test_fixture.rs` around lines 1 - 8, Remove the module-level //! documentation and /// comments on the pub(crate) fixture items in test_fixture.rs, including the referenced locations. Preserve only brief // INVARIANT: comments where the constraint cannot be expressed in code, and add rule-based named tests covering the remaining non-obvious fixture behavior, including runtime skipping and the FIXTURE_REASON distinction from e2e-seed.Source: Coding guidelines
19-23: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDerive
DebugforFixture.Add
#[derive(Debug)]beforeFixture, sinceDatabaseConnection,Uuid, andDatabaseConnectionall support debug formatting.🤖 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/test_fixture.rs` around lines 19 - 23, Add a Debug derive attribute to the Fixture struct so it supports debug formatting, preserving its existing fields and visibility.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/infra/db/test_fixture.rs`:
- Around line 25-35: Update fixture_or_skip and the Fixture lifecycle to clean
up all tenant-scoped rows written with FIXTURE_REASON after each test, covering
persons, org_chart, visibility, and person_roles before releasing the database
connection. Ensure cleanup runs on normal and failing test paths, while
preserving the existing skip behavior when ENV_VAR is unset.
---
Nitpick comments:
In `@src/backend/services/identity-resolution/src/infra/db/test_fixture.rs`:
- Around line 1-8: Remove the module-level //! documentation and /// comments on
the pub(crate) fixture items in test_fixture.rs, including the referenced
locations. Preserve only brief // INVARIANT: comments where the constraint
cannot be expressed in code, and add rule-based named tests covering the
remaining non-obvious fixture behavior, including runtime skipping and the
FIXTURE_REASON distinction from e2e-seed.
- Around line 19-23: Add a Debug derive attribute to the Fixture struct so it
supports debug formatting, preserving its existing fields and visibility.
🪄 Autofix
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: e8cc654c-b491-4749-8dbf-7825907e3d83
⛔ Files ignored due to path filters (1)
src/backend/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
src/backend/services/identity-resolution/Cargo.tomlsrc/backend/services/identity-resolution/src/api/http_live_tests.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/test_fixture.rssrc/backend/services/identity-resolution/src/infra/db/visible_set_live_tests.rssrc/ingestion/tests/e2e/lib/api_coverage.pytests/stand/api/identity/test_visible_persons.py
🚧 Files skipped from review as they are similar to previous changes (8)
- src/backend/services/identity-resolution/Cargo.toml
- src/backend/services/identity-resolution/src/infra/db/mod.rs
- src/ingestion/tests/e2e/lib/api_coverage.py
- src/backend/services/identity-resolution/src/api/mod.rs
- src/backend/services/identity-resolution/src/api/http_live_tests.rs
- tests/stand/api/identity/test_visible_persons.py
- src/backend/services/identity-resolution/src/api/visible_persons.rs
- src/backend/services/identity-resolution/src/infra/db/visible_set_live_tests.rs
| pub(crate) async fn fixture_or_skip() -> anyhow::Result<Option<Fixture>> { | ||
| let Ok(url) = std::env::var(ENV_VAR) else { | ||
| eprintln!("skip: set {ENV_VAR} to run"); | ||
| return Ok(None); | ||
| }; | ||
| Ok(Some(Fixture { | ||
| db: connect_single(&url).await?, | ||
| tenant: Uuid::now_v7(), | ||
| source_id: Uuid::now_v7(), | ||
| })) | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 6 'FIXTURE_REASON|fixture_or_skip\s*\(' .
rg -n -C 6 'DELETE\s+FROM\s+(persons|org_chart|visibility|person_roles)|TRUNCATE\s+' \
src/backend/services/identity-resolutionRepository: constructorfabric/insight
Length of output: 45894
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '--- relevant test_fixture.rs ---\n'
sed -n '1,180p' src/backend/services/identity-resolution/src/infra/db/test_fixture.rs
printf '\n--- repo cleanup/recreate references ---\n'
rg -n -C 4 'INTEGRATION_TESTS_MARIADB_URL|test_fixture|fixtures|e2e|seed|TRUNCATE|RENAME TABLE|DROP TABLE|CREATE DATABASE|CREATE SCHEMA|DELETE FROM|INSERT INTO|sea_orm.*migration|masonite|migration' \
.github src/backend/services/identity-resolution | head -n 240Repository: constructorfabric/insight
Length of output: 254
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo '--- relevant test_fixture.rs ---'
sed -n '1,180p' src/backend/services/identity-resolution/src/infra/db/test_fixture.rs
echo
echo '--- repo cleanup/recreate references ---'
rg -n -C 4 'INTEGRATION_TESTS_MARIADB_URL|test_fixture|fixtures|e2e|seed|TRUNCATE|RENAME TABLE|DROP TABLE|CREATE DATABASE|CREATE SCHEMA|INSERT INTO|sea_orm.*migration|masonite|migration' \
.github src/backend/services/identity-resolution | head -n 320Repository: constructorfabric/insight
Length of output: 37313
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo '--- identity-resolution workflow(s) mentioning mariadb/tests ---'
find .github -type f -iname '*.yml' -o -iname '*.yaml' | xargs -r rg -n -C 5 'identity-resolution|integrat|mariadb|cargo test|docker compose|INTEGRATION_TESTS_MARIADB_URL|seed-persons|TRUNCATE|DROP DATABASE|CREATE DATABASE|RENAME|person_roles|visibility|org_chart' || true
echo
echo '--- fixtures lifecycle and table cleanup in seed_repo.rs around DELETE/TRUNCATE ---'
sed -n '1,380p' src/backend/services/identity-resolution/src/infra/db/seed_repo.rs | rg -n -C 5 'DELETE|RENAME|DROP|CREATE|TRUNCATE|reason|seed|tenant'
echo
echo '--- any delete/fixtures helper methods in db module ---'
rg -n -C 3 'DELETE|TRUNCATE|RENAME|CREATE (DATABASE|SCHEMA)|person_roles|visibility|org_chart|persons|reason\s*=|fixtures|mariadb.*cleanup|cleanup.*mariadb' \
src/backend/services/identity-resolution/. | head -n 220Repository: constructorfabric/insight
Length of output: 50381
Bound the fixture lifetime against reuse.
fixture_or_skip() creates a new tenant, and the live integration DB is reused across tests in ci.yml. The fixture writes persons, org_chart, visibility, and person_roles rows with FIXTURE_REASON, but there is no cleanup path for those rows. Add tenant-scoped cleanup after each fixture or document and enforce per-run DB recreation.
🤖 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/test_fixture.rs` around
lines 25 - 35, Update fixture_or_skip and the Fixture lifecycle to clean up all
tenant-scoped rows written with FIXTURE_REASON after each test, covering
persons, org_chart, visibility, and person_roles before releasing the database
connection. Ensure cleanup runs on normal and failing test paths, while
preserving the existing skip behavior when ENV_VAR is unset.
Source: Coding guidelines
3 participants
Why
Auditing the test coverage of the
person_idcutover (#2098) turned up onelopsided half: analytics landed at 92.9% on new code, while the identity
service's API layer was effectively untested.
api/handlers.rssat at 3.9%line coverage and
persons_repo::person_existshad no test at all.The repo-level
visible_set_live_testsreach the SQL, but nothing exercisedextractor → gate → handler. So the status codes that analytics and the SPA
depend on — the 400/401/404 matrix, and the tenant bound on the wildcard
branch — were unpinned. Identity is also
cover: falsein the CI componentregistry, so no gate would have noticed.
Tests only. No production code changes beyond one constant's visibility.
What
api/http_live_tests.rs(new) drives the real route table throughtower::oneshotagainst a live MariaDB — the same harness shape the analyticsservice uses:
POST /v1/visible-persons— the subtree answer; 400 for a request namingnobody (empty / all-nil / non-UUID / over the cap); 401 without an identified
caller; and the wildcard branch: a wildcard grant covers everyone in the
tenant, so a foreign or invented id must not come back confirmed. Analytics
reads this answer as authorization, so that bound is now pinned at the layer
analytics consumes it — previously it was only covered one level down, and
the branch was never taken in any e2e run.
POST /v1/profileswithvalue_type='person_id'— resolution; a person withno email (the case the key change exists for); 404 outside the visible set
and 200 once granted; 404 cross-tenant; 404 unknown; 400 for an email, the nil
UUID, or source fields.
infra/db/test_fixture.rs(new) holds the fixture both suites now share,instead of a second copy drifting from the first. It gains
in_another_tenant()and
emailless_person().person_existsgets two live tests: existence is tenant-bound, and a personwith no email still exists.
e2e —
POST /v1/visible-personsgains its 400 (empty, nil, over-cap) and401 cases, and joins
IDENTITY_RUST_REQUIRED_EXTRAfor{400, 401}. The specdeclares only
200for this route, so until now the gate asked nothing of itbeyond "some test called it once".
Not
#[ignore]d, on purposeThe identity CI job runs plain
cargo test(no--include-ignored), so anignored case here silently stops running. These self-skip on a missing
INTEGRATION_TESTS_MARIADB_URL, like every other live suite in this crate. Theinvariant is stated at the top of both files.
Evidence
cargo test -p identity-resolutionagainst a live MariaDB: 105 passed, 0failed.
cargo clippy --all-targets --all-features: clean.cargo fmt:clean.
./e2e.sh test identity/: 145 passed, 3 skipped.98.6% of coverable codes observed.
persons_in_tenantfails both the repo-level and the new HTTP-level test.Removing the observed 400/401 from the ledger fails the gate with
MISSING REQUIRED_EXTRA: POST /v1/visible-persons never answered [400, 401].Coverage of the touched files:
api/handlers.rsapi/visible_persons.rsinfra/db/persons_repo.rsFollow-ups, not in this PR
cover: falsefor identity-resolution inscripts/ci/components.pystillcannot be flipped: 55.1% is under the 80% floor. The remaining weight is
outside the cutover (
visibility_repo.rs0%,sync_runner.rs12%,subchart_repo.rs37%).api/metric_drilldown.rsmeasured 18.2% on new code (theauthorize_entity_idscall has no test), and
POST /v1/metrics/{id}/query/POST /v1/metrics/querieshave their
403marked as boilerplate inapi_coverage.py, so the gate cannotask for it.
infra/db/mod.rscountsinformation_schema.CHECK_CONSTRAINTSwithout a schema filter, so it fails onany MariaDB server hosting a second identity schema beside
identity. Nottouched here.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests