docs(identity-resolution): sync domain spec with the implemented journal architecture + ADR-0003 - #2234
Conversation
|
Warning Review limit reached
Next review available in: 21 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe PR replaces the legacy alias-resolution design with a MariaDB ChangesIdentity resolution journal
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
722a4df to
e2921b8
Compare
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (4)
src/backend/services/identity-resolution/src/api/http_live_tests.rs (1)
33-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the required test result alias.
Line 33 uses
TestResult = anyhow::Result<()>. Usetype R = Result<(), Box<dyn Error>>and update the test return types toR.As per coding guidelines, “alias
type R = Result<(), Box<dyn Error>>to reduce ceremony.”🤖 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` at line 33, Replace the TestResult alias with the required R alias using Result<(), Box<dyn Error>>, then update all test function return types that reference TestResult to use R instead.Source: Coding guidelines
src/backend/services/identity-resolution/src/infra/db/test_fixture.rs (2)
1-8: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove non-compliant service test comments.
Keep only a one-line
// INVARIANT:comment when code and test names cannot state the reason. Remove the documentation headers, section banners, and redundant rationale comments in the changed test code.
src/backend/services/identity-resolution/src/infra/db/test_fixture.rs#L1-L8: Replace the documentation header with only essential one-line invariants.src/backend/services/identity-resolution/src/api/http_live_tests.rs#L1-L13: Remove the module documentation header and apply the same rule to the changed section banners and rationale comments.src/backend/services/identity-resolution/src/infra/db/visible_set_live_tests.rs#L86-L88: Remove the rationale comment because the test name states the rule; apply the same change to the emailless-person comment.As per coding guidelines, “Use comments only when code cannot express the reason” 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 documentation header from test_fixture.rs, retaining only essential one-line INVARIANT comments whose reasons cannot be expressed by code or test names. In http_live_tests.rs, remove the module documentation header, section banners, and redundant rationale comments in the changed sections. In visible_set_live_tests.rs, remove the rationale comments for the named rule and emailless-person cases; update all three sites as specified.Source: Coding guidelines
19-23: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDerive
Debugfor test support types.
src/backend/services/identity-resolution/src/infra/db/test_fixture.rs#L19-L23: Add#[derive(Debug)]toFixture.src/backend/services/identity-resolution/src/api/http_live_tests.rs#L35-L40: AddDebugtoCaller’s derive list.Verify that
DatabaseConnectionimplementsDebugbefore deriving it forFixture. As per coding guidelines, “DeriveDebugfor types.”🤖 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, Derive Debug for the test support types: add Debug to Fixture in src/backend/services/identity-resolution/src/infra/db/test_fixture.rs:19-23 after confirming DatabaseConnection implements Debug, and add Debug to Caller’s derive list in src/backend/services/identity-resolution/src/api/http_live_tests.rs:35-40.Source: Coding guidelines
src/ingestion/tests/e2e/lib/api_coverage.py (1)
120-125: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueKeep changed Python comments to one line.
The test names and assertions already state the validation rules. Remove the extended rationale or reduce it to one line.
src/ingestion/tests/e2e/lib/api_coverage.py#L120-L125: Replace the five-line coverage rationale with one concise contract comment.tests/stand/api/identity/test_visible_persons.py#L96-L108: Remove or reduce the multi-line test docstrings to one line.As per coding guidelines, “Add comments only when code cannot express the reason” and “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 multi-line coverage rationale in src/ingestion/tests/e2e/lib/api_coverage.py lines 120-125 to one concise contract comment for POST /v1/visible-persons. Also remove or reduce the multi-line test docstrings in tests/stand/api/identity/test_visible_persons.py lines 96-108 to one line each, preserving the existing test names and assertions.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
`@docs/domain/identity-resolution/specs/ADR/0002-stable-person-id-via-persons-observations.md`:
- Around line 9-12: Update the ADR’s later schema and cache-contract sections to
match the current contracts documented in DESIGN.md: replace stale
value_hash/utf8mb4_bin/TIMESTAMP(6) definitions with created_at,
case-insensitive value_id, and DATETIME(6), and revise or explicitly mark the
RENAME TABLE cache-swap behavior accordingly. Ensure the ADR has a single
consistent normative schema while preserving the stable person_id, append-only
observations, and derived account_person_map decisions.
In
`@docs/domain/identity-resolution/specs/ADR/0003-operator-decisions-as-persons-observations.md`:
- Around line 58-60: The account-linkage contract is inconsistent across the
operator-decision documents. In
docs/domain/identity-resolution/specs/ADR/0003-operator-decisions-as-persons-observations.md
lines 58-60, clarify that value_type='id' binding observations identify the
account through source_account_id stored in value_id, while only non-id
attributes require identity_inputs; apply the same distinction to operator
corrections in docs/domain/identity-resolution/specs/DESIGN.md lines 1036-1040.
In `@docs/domain/identity-resolution/specs/DECOMPOSITION.md`:
- Around line 161-162: Update the `persons` entry in the decomposition
specification to say “append observations” instead of describing it as an update
or fold operation, preserving the append-only semantics defined by ADR-0002 and
`DESIGN.md`.
In `@docs/domain/identity-resolution/specs/DESIGN.md`:
- Around line 924-926: Remove the blank line between the `Source` and `Status`
lines in the blockquote, keeping both quoted lines adjacent in the status
section of `DESIGN.md`.
- Line 293: Update the architecture diagram code fence at the specified location
in DESIGN.md to declare the text language, using the existing box-drawing
diagram content unchanged.
- Line 835: Update the no-delete rule in DESIGN.md to state that the seed never
deletes or updates rows in persons, while explicitly permitting the
tenant-scoped delete-and-rebuild of derived account_person_map within the
transaction described by Rebuild.
- Around line 863-865: Update the data-flow description for reading
identity.identity_inputs so the non-empty value requirement applies only to
UPSERT rows; retain DELETE rows even when value is empty and treat them as
closure signals rather than persisted observations.
In `@docs/domain/identity-resolution/specs/PRD.md`:
- Around line 557-575: Update the later Person Domain Cross-Reference Contract
to remove the legacy aliases.person_id authority and person-creation claims.
Align it with the Analytics Resolution Interface, DESIGN.md, and ADR-0002:
identity-resolution mints person_id, persons is authoritative, and analytics
resolves through identity.identity_persons and resolve_person_id.
- Around line 738-741: Update the acceptance criterion about accounts observed
by connectors to align with the documented e-mail-less behavior: either scope
the binding/review requirement to accounts with e-mail addresses, or revise the
surrounding requirements so e-mail-less accounts are also surfaced for review.
Preserve the existing seed resolution and review semantics for the selected
scope.
In `@src/backend/services/identity-resolution/src/api/http_live_tests.rs`:
- Around line 90-95: Update the test helper post to avoid unbounded response
buffering by defining a module-level MAX_RESPONSE_BYTES constant and passing it
to to_bytes instead of usize::MAX. Set the cap above the largest valid response
exercised by these tests, preserving existing parsing and return behavior.
---
Nitpick comments:
In `@src/backend/services/identity-resolution/src/api/http_live_tests.rs`:
- Line 33: Replace the TestResult alias with the required R alias using
Result<(), Box<dyn Error>>, then update all test function return types that
reference TestResult to use R instead.
In `@src/backend/services/identity-resolution/src/infra/db/test_fixture.rs`:
- Around line 1-8: Remove the documentation header from test_fixture.rs,
retaining only essential one-line INVARIANT comments whose reasons cannot be
expressed by code or test names. In http_live_tests.rs, remove the module
documentation header, section banners, and redundant rationale comments in the
changed sections. In visible_set_live_tests.rs, remove the rationale comments
for the named rule and emailless-person cases; update all three sites as
specified.
- Around line 19-23: Derive Debug for the test support types: add Debug to
Fixture in
src/backend/services/identity-resolution/src/infra/db/test_fixture.rs:19-23
after confirming DatabaseConnection implements Debug, and add Debug to Caller’s
derive list in
src/backend/services/identity-resolution/src/api/http_live_tests.rs:35-40.
In `@src/ingestion/tests/e2e/lib/api_coverage.py`:
- Around line 120-125: Shorten the multi-line coverage rationale in
src/ingestion/tests/e2e/lib/api_coverage.py lines 120-125 to one concise
contract comment for POST /v1/visible-persons. Also remove or reduce the
multi-line test docstrings in tests/stand/api/identity/test_visible_persons.py
lines 96-108 to one line each, preserving the existing test names and
assertions.
🪄 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: 2c00619b-427a-47fb-a2c8-e8b1941bfcb6
📥 Commits
Reviewing files that changed from the base of the PR and between 4c96af2 and 722a4dfe127ee3525e0e8a058d44da3e84d0167a.
⛔ Files ignored due to path filters (1)
src/backend/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (15)
.cf-studio/config/artifacts.tomldocs/domain/identity-resolution/specs/ADR/0002-stable-person-id-via-persons-observations.mddocs/domain/identity-resolution/specs/ADR/0003-operator-decisions-as-persons-observations.mddocs/domain/identity-resolution/specs/DECOMPOSITION.mddocs/domain/identity-resolution/specs/DESIGN.mddocs/domain/identity-resolution/specs/PRD.mdsrc/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
| - `persons` → (deterministic rebuild) → `account_person_map` | ||
| - `persons` → (persons-sync atomic swap) → `identity.identity_persons` → (dbt `resolve_person_id`) → gold `person_id` columns | ||
| - Operator corrections (ADR-0003) → append to `persons`; payloads journaled in `operations` | ||
| - `persons.person_id` is the stable cross-domain join key (random UUIDv7, never re-derived; ADR-0002) | ||
|
|
||
| ### 3.2 Component Model | ||
|
|
||
| ``` |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Add a language tag to the architecture diagram fence.
markdownlint MD040 reports the fence at Line 293. Use text for the box-drawing diagram.
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 293-293: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 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/domain/identity-resolution/specs/DESIGN.md` at line 293, Update the
architecture diagram code fence at the specified location in DESIGN.md to
declare the text language, using the existing box-drawing diagram content
unchanged.
Source: Linters/SAST tools
| DROP TABLE account_person_map_old; | ||
| ``` | ||
| The `RENAME TABLE` pair is atomic in MariaDB; concurrent readers see either the old or the new map, never an empty intermediate. | ||
| - **Rebuild** (at the end of every seed run + every future operator flow) — **transactional, tenant-scoped**: within the same transaction that applies the observations, the seed issues `DELETE FROM account_person_map WHERE insight_tenant_id = ?` followed by `INSERT ... SELECT ... LEAD() OVER (PARTITION BY tenant, source_type, source_id, account ORDER BY created_at)` from `persons.value_type='id'` rows. The journal write and the cache rebuild commit atomically — readers see either the pre-run or the post-run state, and the log and cache are never observably inconsistent. (An earlier design described a `RENAME TABLE` two-table swap; the implemented mechanism is the transactional delete-and-insert above.) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Scope the no-delete rule to persons.
The rebuild at Line 835 deletes rows from derived account_person_map. The document also says that the seed never issues DELETE, while later steps describe this cache delete. State that the seed never deletes or updates persons; allow the tenant-scoped cache rebuild inside the transaction.
🤖 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/domain/identity-resolution/specs/DESIGN.md` at line 835, Update the
no-delete rule in DESIGN.md to state that the seed never deletes or updates rows
in persons, while explicitly permitting the tenant-scoped delete-and-rebuild of
derived account_person_map within the transaction described by Rebuild.
| **Process** (data flow executed by each seed run): | ||
|
|
||
| 1. Read all rows from `identity.identity_inputs` (ClickHouse) where `operation_type = 'UPSERT'` and `value` is non-empty. Order by `_synced_at DESC` within each source-account so that the latest email observation is picked deterministically in step 5. | ||
| 1. Read `identity.identity_inputs` (ClickHouse): UPSERT and DELETE rows with non-empty `value` (DELETE is a closure signal only, never persisted). **The full set each run** — no incremental watermark yet (REC-IR-02) — and currently without a tenant predicate (single-tenant deployments; multi-tenant prerequisite). Order by `_synced_at DESC` within each source-account so that the latest email observation is picked deterministically in step 5. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not filter out empty-value DELETE rows.
Line 640 defines deactivation DELETE rows with empty value, but Line 865 requires non-empty value for both UPSERT and DELETE. That drops closure signals and can leave deactivated account observations active. Keep non-empty UPSERT rows and keep DELETE rows as closure events.
🤖 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/domain/identity-resolution/specs/DESIGN.md` around lines 863 - 865,
Update the data-flow description for reading identity.identity_inputs so the
non-empty value requirement applies only to UPSERT rows; retain DELETE rows even
when value is empty and treat them as closure signals rather than persisted
observations.
| > Source: `inbox/IDENTITY_RESOLUTION.md` | ||
|
|
||
| This is an **alternative implementation** of identity grouping — runs entirely in ClickHouse on `(token, rid)` pairs. The primary architecture uses BootstrapJob + MatchingEngine for incremental resolution; the min-propagation algorithm may be used for bulk initial grouping or as a verification tool to detect grouping inconsistencies. | ||
| > **Status: future — not implemented.** Kept as candidate material for the matcher iteration (bulk verification / candidate generation). Note two ADR-0003 constraints on any future use: transitive auto-grouping must never write bindings (proposals only), and operator decisions in the journal override its output. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the blank line inside the status blockquote.
markdownlint MD028 reports Line 925. Keep the Source and Status quote lines adjacent, or prefix the blank line with >.
🧰 Tools
🪛 markdownlint-cli2 (0.23.2)
[warning] 925-925: Blank line inside blockquote
(MD028, no-blanks-blockquote)
🤖 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/domain/identity-resolution/specs/DESIGN.md` around lines 924 - 926,
Remove the blank line between the `Source` and `Status` lines in the blockquote,
keeping both quoted lines adjacent in the status section of `DESIGN.md`.
Source: Linters/SAST tools
| async fn post(app: Router, uri: &str, body: &Value) -> anyhow::Result<(StatusCode, Value)> { | ||
| let resp = app.oneshot(json_req(uri, body)?).await?; | ||
| let status = resp.status(); | ||
| let bytes = to_bytes(resp.into_body(), usize::MAX).await?; | ||
| let payload = serde_json::from_slice(&bytes).unwrap_or(Value::Null); | ||
| Ok((status, payload)) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate file =="
git ls-files | rg '(^|/)src/backend/services/identity-resolution/src/api/http_live_tests\.rs$' || true
echo "== file outline / relevant sections =="
if [ -f src/backend/services/identity-resolution/src/api/http_live_tests.rs ]; then
wc -l src/backend/services/identity-resolution/src/api/http_live_tests.rs
sed -n '1,180p' src/backend/services/identity-resolution/src/api/http_live_tests.rs
echo "== search to_bytes usages =="
rg -n "to_bytes|MAX_RESPONSE|MAX_|::MAX|app\.oneshot|json_req" src/backend/services/identity-resolution/src/api/http_live_tests.rs
fiRepository: constructorfabric/insight
Length of output: 220
Bound the buffered response body.
to_bytes(resp.into_body(), usize::MAX) can allocate an unbounded response body if a route changes its behavior. Define a module-level MAX_RESPONSE_BYTES cap and pass it here; if this test currently only exercises small valid endpoint responses, keep the cap above the maximum valid response size.
🤖 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 90 - 95, Update the test helper post to avoid unbounded response buffering
by defining a module-level MAX_RESPONSE_BYTES constant and passing it to
to_bytes instead of usize::MAX. Set the cap above the largest valid response
exercised by these tests, preserving existing parsing and return behavior.
Source: Coding guidelines
|
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: 9
♻️ Duplicate comments (1)
docs/domain/identity-resolution/specs/DECOMPOSITION.md (1)
162-163: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winKeep
personsappend-only.ADR-0002 defines
personsas an append-only observation journal.account_person_mapis the derived cache rebuilt from those observations. Replaceupdate — fold new observationswithappend observations; keep folding as the derivation step.🤖 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/domain/identity-resolution/specs/DECOMPOSITION.md` around lines 162 - 163, Update the persons entry in the documented table list to describe appending observations, not updating or folding records. Keep folding explicitly associated with the derived account_person_map cache rebuilt from the persons observation journal.
🤖 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/domain/identity-resolution/specs/DECOMPOSITION.md`:
- Around line 77-79: Update the coverage checklist entries for
cpt-insightspec-ir-principle-alias-centric and cpt-ir-fr-manual-alias-crud to
mark them as superseded or move them into a historical traceability section.
Ensure the Feature 4 mapping preserves their legacy status and excludes both
alias artifacts from current coverage.
- Around line 263-264: Expand the excluded-person behavior specification in
DECOMPOSITION.md to cover the resolve macro, service read API, person domain,
and review queue. For each consumer, define the reserved sentinel as “no person”
and ensure excluded accounts cannot remain visible as persons on non-dbt paths,
while preserving the existing macro mapping to NULL.
- Around line 329-334: Remove the “proposals (with the matcher)” entry from the
Late-phase items list in DECOMPOSITION.md, since proposal generation, storage or
derivation, and review are already decomposed. Retain only genuinely deferred
components, such as stored negative rules, value blocklists, or future GDPR
alias deletion.
- Around line 121-127: Update DECOMPOSITION.md to describe the tenant-scoped,
audited POST /v1/persons-seed endpoint as the implemented persons-seed entry
point, replacing references to a scheduled service subcommand and any statement
that no seed API exists. Mark the Python script as an emergency fallback, revise
the Purpose, Scope, and API sections accordingly, and document any scheduler
invoking the endpoint separately rather than presenting it as the entry point.
- Line 28: Update the dependency documentation to include the Feature 4 →
Feature 3 edge wherever dependencies are described: the overview at the changed
dependency summary, the graph around the dependency diagram, and the rationale
sections near the cited lines. Ensure the implementation order reflects that
Feature 3’s proposal acceptance uses the Feature 4 API, while preserving the
existing dependencies and no-circular-dependencies statement.
In `@docs/domain/identity-resolution/specs/DESIGN.md`:
- Around line 867-871: Update step 4’s existing-email load to read current
effective email rows with their person_id, rather than collecting only (tenant,
normalized_email) values from the historical journal. Build a
tenant/email-to-person mapping, mark emails associated with multiple persons as
contested, and ensure step 5’s LinkedByEmail path auto-links only unambiguous
mappings while surfacing contested values.
- Around line 876-879: Update the observation routing rules in the INSERT IGNORE
seed logic so value_type values of id, email, and username populate value_id.
Preserve the existing display_name and catch-all routing behavior for all other
value types.
- Line 779: The persons-seed write step must require strictly increasing
DATETIME(6) timestamp allocation per affected account/row when deriving
created_at from identity_inputs._synced_at, matching the operator-path
obligation. Add this obligation to DESIGN.md and mirror the corresponding
requirement in ADR-0003-operator-decisions-as-persons-observations.md; both
sites require direct documentation updates.
In `@docs/domain/identity-resolution/specs/PRD.md`:
- Around line 413-417: Update the cpt-ir-fr-correction-durability checklist item
in PRD.md to mark correction durability as target behavior or blocked rather
than an unconditional implemented guarantee, consistent with DESIGN.md’s current
seed limitations. Preserve the requirement text while clearly indicating it is
not an acceptance criterion until divergent-group seed hardening prevents
account rebinding.
---
Duplicate comments:
In `@docs/domain/identity-resolution/specs/DECOMPOSITION.md`:
- Around line 162-163: Update the persons entry in the documented table list to
describe appending observations, not updating or folding records. Keep folding
explicitly associated with the derived account_person_map cache rebuilt from the
persons observation journal.
🪄 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: ceec55e0-9664-4abf-a9bc-e354c5cac365
📥 Commits
Reviewing files that changed from the base of the PR and between 4c96af2 and 9223e4f1790d7285b7911ff07e8eae3bcee1c866.
📒 Files selected for processing (6)
.cf-studio/config/artifacts.tomldocs/domain/identity-resolution/specs/ADR/0002-stable-person-id-via-persons-observations.mddocs/domain/identity-resolution/specs/ADR/0003-operator-decisions-as-persons-observations.mddocs/domain/identity-resolution/specs/DECOMPOSITION.mddocs/domain/identity-resolution/specs/DESIGN.mddocs/domain/identity-resolution/specs/PRD.md
🚧 Files skipped from review as they are similar to previous changes (2)
- .cf-studio/config/artifacts.toml
- docs/domain/identity-resolution/specs/ADR/0002-stable-person-id-via-persons-observations.md
| - **Purpose**: Give connectors one uniform write target for identity observations and fold that evidence into the journal automatically. Connectors populate `identity_inputs` through dbt (`identity_inputs_from_history` macro, incremental append models); the scheduled persons-seed groups accounts by e-mail and binds them (reuse / link-by-e-mail / mint / skip, never merging existing persons); persons-sync republishes the journal to ClickHouse for analytics. | ||
|
|
||
| - **Depends On**: `cpt-ir-feature-initial-seed` (aliases table and Resolution API must exist) | ||
| - **Depends On**: `cpt-ir-feature-initial-seed` (the `persons` journal and read paths must exist) | ||
|
|
||
| - **Scope**: | ||
| - Create `identity_inputs` table in ClickHouse | ||
| - Create `unmapped` table for unresolved aliases | ||
| - Create `conflicts` table for alias-level disagreements | ||
| - BootstrapJob: reads identity_inputs incrementally (`_synced_at > last_watermark`). See DESIGN §5 REC-IR-02 for recommended watermark mechanism (dbt incremental + `bootstrap_watermarks` table) | ||
| - Alias normalization: email/username → `lower(trim())`; others → `trim()` | ||
| - Auto-create alias on exact match (confidence >= 1.0 from direct lookup) | ||
| - Route unresolved aliases to `unmapped` table | ||
| - Detect alias conflicts when same alias claimed by different persons | ||
| - Track `last_observed_at` for existing aliases | ||
| - Auto-resolve unmapped entries when matching aliases are created | ||
| - Idempotent bootstrap runs (dedup on natural key) | ||
| - Argo Workflow integration for scheduling | ||
| - `identity_inputs` table and write contract; per-connector dbt models via the shared macro (incremental `append` on `_synced_at`) | ||
| - persons-seed as a scheduled service subcommand: run-lock, input guards with explicit `--force`, run journal in `operations` with per-branch counters |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Describe the implemented seed entry point.
The supplied implementation context says the algorithm moved to the tenant-scoped, audited POST /v1/persons-seed endpoint. The Python script is an emergency fallback. This file still describes a scheduled service subcommand and says that no seed API exists. Update the purpose, scope, and API sections. If a scheduler invokes the endpoint, document that caller separately.
Also applies to: 170-172
🤖 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/domain/identity-resolution/specs/DECOMPOSITION.md` around lines 121 -
127, Update DECOMPOSITION.md to describe the tenant-scoped, audited POST
/v1/persons-seed endpoint as the implemented persons-seed entry point, replacing
references to a scheduled service subcommand and any statement that no seed API
exists. Mark the Python script as an emergency fallback, revise the Purpose,
Scope, and API sections accordingly, and document any scheduler invoking the
endpoint separately rather than presenting it as the entry point.
| @@ -763,7 +776,7 @@ Exactly one of `(value_id, value_full_text, value)` is populated per normal row; | |||
| - `idx_person_id (person_id)` — list all attributes for a person | |||
| - `idx_tenant_person (insight_tenant_id, person_id)` — tenant-scoped person lookup | |||
| - `idx_source (insight_source_type, insight_source_id)` — filter by source system + instance | |||
| - `uq_person_observation (insight_tenant_id, person_id, insight_source_type, insight_source_id, value_type, value_hash)` UNIQUE — enforces the natural observation key. The generated `value_hash` column (SHA-256 hex of the coalesced value) gives a fixed-width, collision-free discriminator regardless of value length, which is required because (a) MariaDB treats `NULL` as distinct in UNIQUE keys and (b) catch-all `TEXT` values cannot be fully indexed by prefix without truncation collisions. Combined with `INSERT IGNORE` in the seed, this guarantees idempotent re-runs | |||
| - `uq_person_observation (insight_tenant_id, person_id, insight_source_type, insight_source_id, value_type, created_at)` UNIQUE — the natural observation key as of migration 004. `created_at` (taken from the observation's `identity_inputs._synced_at` for seed writes) disambiguates repeated observations: re-emission of the same observation at the same `created_at` collapses via `INSERT IGNORE` (seed re-run idempotency), while a genuine later re-observation of the same value is a new history row. Two obligations follow for writers: (a) the key does **not** deduplicate a re-applied operator correction (its `created_at` is new) — correction idempotency is decision-aware at the API level (§3.3); (b) the key contains **no account discriminator** — two `value_type='id'` observations for two different accounts of the same source, bound to the same person at the same `created_at`, collide and `INSERT IGNORE` silently drops one. The correction write path MUST therefore allocate strictly increasing `DATETIME(6)` timestamps per affected row within an operation (bulk merge/bind included); extending the key with an account discriminator is a candidate follow-up migration | |||
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Find relevant files:"
fd -a 'DESIGN\.md|0003-operator-decisions-as-persons-observations\.md|migration|migrations' . | sed 's#^\./##' | head -100
echo
echo "Search natural key and _synced_at usage:"
rg -n "uq_person_observation|identity_inputs|_synced_at|INSERT IGNORE|person_observation|DATETIME\(6\)|natural observation key|account discriminator" .
echo
echo "Show relevant docs lines:"
for f in docs/domain/identity-resolution/specs/DESIGN.md docs/domain/identity-resolution/specs/ADR/0003-operator-decisions-as-persons-observations.md; do
if [ -f "$f" ]; then
echo "===== $f ====="
wc -l "$f"
sed -n '740,800p' "$f"
echo "-----"
sed -n '45,85p' "$f"
fi
done
echo
echo "Search schema/seed/migration references around person observation inserts:"
rg -n "INSERT INTO .*person_observation|INSERT IGNORE|person_observation|uq_person_observation|_synced_at|created_at" docs migrations src tables app packages --glob '!node_modules' --glob '!dist' --glob '!build' 2>/dev/null || true
echo
echo "Git status/stat:"
git status --short
git diff --statRepository: constructorfabric/insight
Length of output: 50382
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Relevant identity resolution spec sections"
sed -n '720,900p' docs/domain/identity-resolution/specs/DESIGN.md
echo
sed -n '45,80p' docs/domain/identity-resolution/specs/ADR/0003-operator-decisions-as-persons-observations.md
echo
echo "Search seed writer calls and person insert helpers"
rg -n "INSERT IGNORE|uq_person_observation|person_observation|value_type='id'|person_id|insight_source_account_id|source_account_id|created_at|_synced_at|authors|author_person_id" src backend --glob '!node_modules' --glob '!target' --glob '!build' 2>/dev/null | head -200
echo
echo "Find persons observation repository/save files"
fd -i 'persons.*|identity.*|seed.*|person' src backend | head -100Repository: constructorfabric/insight
Length of output: 50381
Require timestamp uniqueness for persons-seed writes.
Seed binding rows derive created_at from identity_inputs._synced_at, while uq_person_observation omits the account discriminator. If two accounts from the same source resolve to one person at the same _synced_at, the seed needs per-account timestamp allocation like the operator path. Add this obligation under the seed write step and mirror it in ADR-0003.
📍 Affects 2 files
docs/domain/identity-resolution/specs/DESIGN.md#L779-L779(this comment)docs/domain/identity-resolution/specs/ADR/0003-operator-decisions-as-persons-observations.md#L71-L71
🤖 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/domain/identity-resolution/specs/DESIGN.md` at line 779, The
persons-seed write step must require strictly increasing DATETIME(6) timestamp
allocation per affected account/row when deriving created_at from
identity_inputs._synced_at, matching the operator-path obligation. Add this
obligation to DESIGN.md and mirror the corresponding requirement in
ADR-0003-operator-decisions-as-persons-observations.md; both sites require
direct documentation updates.
| 3. Connect to MariaDB. Load known bindings: for each source-account key, find the latest `value_type='id'` observation in `persons` and capture its `person_id`. This becomes the **known-account** set. | ||
| 4. Load existing emails: run `SELECT insight_tenant_id, LOWER(TRIM(value_id)) FROM persons WHERE value_type='email' AND value_id IS NOT NULL AND value_id != ''` and collect into a `(tenant, normalized_email)` set. The set is empty on the very first run (initial bootstrap) and non-empty afterwards; the same code path handles both — there is no mode flag. | ||
| 5. For each source-account in `identity_inputs`: | ||
| - **Known account** (present in step 3 set): reuse the mapped `person_id`. Observations go to `persons` via `INSERT IGNORE` (dedupe on UNIQUE key); no new binding. | ||
| - **Unknown account, no email observed**: skip. Email remains the sole identity anchor for this seed. | ||
| - **Unknown account, email absent from step 4 set**: mint a new `person_id` (random UUIDv7). `reason=''`. Within the same run, two new accounts sharing this new email still share one `person_id` (email-automerge within the run); on a fresh-tenant run this is the initial-bootstrap behaviour as a special case. | ||
| - **Unknown account, email already present in step 4 set**: **mint a fresh isolated `person_id`** (visibly NOT merged with the existing email-bearer) and write all observations with `reason='pending-iresolution'`. Each pending account gets its own `person_id` (no intra-run automerge among pending accounts), so the future Identity-Resolution operator flow has per-account granularity. The IRes flow scans for `reason='pending-iresolution'` rows and prompts a per-account decision (link to existing email-bearer / keep separate / merge). | ||
| 5. Group accounts by normalized current e-mail; resolve each group in priority order (mirrors the .NET resolver; `domain/seed.rs::resolve_assignments`): | ||
| - **Group with a bound account** (step 3 set): reuse that `person_id` for the whole group; no new binding decision. **Known gap**: if the group's accounts are bound to *different* persons, the group currently collapses onto the first binding (counted in `known_binding_conflicts`, logged) and can thereby silently re-derive a binding — the manual-resolution hardening replaces this with per-account binding respect and surfacing. | ||
| - **Unbound group, e-mail present in step 4 set**: link the group to that person (`LinkedByEmail`) — the account joins an existing person automatically when the e-mail is unambiguous. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Load a current e-mail-to-person mapping.
Line 868 returns only (tenant, email), but Line 871 must select a person_id. The query also reads historical rows from the append-only journal. A stale e-mail can link a new account to the wrong person, and a contested e-mail cannot be detected. Load current effective e-mail rows with person_id; surface values mapped to multiple persons instead of auto-linking them.
🤖 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/domain/identity-resolution/specs/DESIGN.md` around lines 867 - 871,
Update step 4’s existing-email load to read current effective email rows with
their person_id, rather than collecting only (tenant, normalized_email) values
from the historical journal. Build a tenant/email-to-person mapping, mark emails
associated with multiple persons as contested, and ensure step 5’s LinkedByEmail
path auto-links only unambiguous mappings while surfacing contested values.
| 6. Write observations to `persons` via `INSERT IGNORE`. Routing rules (hardcoded in the seed, mirrored by the dbt macro): | ||
| - `value_type IN ('id', 'email')` → `value_id = value`, others NULL | ||
| - `value_type = 'display_name'` → `value_full_text = value`, others NULL | ||
| - otherwise → `value = value`, others NULL |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Route username observations to value_id.
Lines 749 and 763 define id, email, and username as value_id values. The seed branch at Line 877 omits username, so username rows go to catch-all value and bypass the indexed lookup path. Add username to the value_id branch.
🤖 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/domain/identity-resolution/specs/DESIGN.md` around lines 876 - 879,
Update the observation routing rules in the INSERT IGNORE seed logic so
value_type values of id, email, and username populate value_id. Preserve the
existing display_name and catch-all routing behavior for all other value types.
…nal architecture The domain spec (v2.0) described a ClickHouse-native alias pipeline (aliases store, BootstrapJob/MatchingEngine/ResolutionService, unmapped/ conflicts/merge_audits tables) that was never built. Sync it with what actually runs, and record the manual-resolution decision: - DESIGN v3.0: journal architecture (identity_inputs evidence -> persons-seed fold -> append-only persons journal -> persons-sync mirror -> dbt resolve macro); implemented components PersonsSeed / PersonsSync / IdentityReadApi and planned OperatorResolutionApi; real sequences (seed run, operator correction, build resolution); unbuilt tables compressed to future-table summaries; aliases marked legacy; schema synced to service migrations 004/009/014; excluded- person sentinel defined; account-first resolver upgrade specified (source-instance-scoped account key, account-derived e-mail fallback, contested e-mail -> NULL); known gaps documented (divergent-group collapse, full-set fold without watermark, no tenant predicate on the evidence read). - New ADR-0003 (proposed): operator corrections as append-only persons observations — no separate decision store in v1; required enablers (resolver upgrade, seed hardening, decision-aware idempotency, unique per-row observation timestamps) and revisit triggers. - PRD: implementation-neutral -v2 requirements for merge/split/audit/ idempotency plus new operator-correction requirements (bind incl. bulk and pre-registration, exclude, review queue, correction durability, binding history); superseded alias-pipeline requirements marked and retained for traceability; bootstrap-incremental unchecked (full-set fold today, watermark open). - DECOMPOSITION: features rewritten around the shipped architecture; new entry 2.4 Manual Resolution; future matcher reframed as proposals-only. - artifacts.toml: register domain ADR-0002/0003; ADR-0002 gains an ID line and a post-acceptance schema note. All artifacts pass cfs validate / validate-toc / check-language. Known debt: ADR-0002 predates the template and keeps its original section structure (accepted ADRs are not restructured). Design context: #2180 (reviewed operator-correction design), #1873. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Sergei Mozhaev <mozhaev.dev@gmail.com>
…rge contracts Review-queue contract made buildable: the queue is normatively derived from TWO sources joined on the account key - identity_inputs evidence (every observed account, including e-mail-less ones) and current persons bindings - across the endpoint description, ADR-0003, principles, drivers, walkthrough and observability. E-mail-less accounts are never hidden: they surface as no-evidence queue items awaiting an operator bind (#1776), and the queue reports resolution-rate shares (bound / pending / no-evidence / excluded) - the reported-match-rate success measure of #1873. GDPR purge contract rewritten: erasure is an explicit administrative operation outside the decision journal (recorded in operations) that erases identity values everywhere they rest - persons value payloads, identity_inputs evidence, legacy aliases - with no plaintext copy retained anywhere; value-free tombstones (e.g. salted hashes) form a re-link deny-list. The v2.0 plaintext alias_gdpr_deleted archive is rejected as contradicting hard erasure (it also never touched persons). Append-only is scoped to identity decisions and does not preclude lawful erasure. Also from the PM review: reviewer namespace named explicitly (glossary + account-first resolver); ignore/defer recorded as a deliberate MVP narrowing (returns with the proposal store); operator authorization pinned to the existing roles/person_roles grants; confidence/evidence reserved as column names for the matcher iteration with the deviation from the physical-columns suggestion recorded explicitly; admin identity console fixed as the next step after the operator API stabilizes. All artifacts pass cfs validate / validate-toc / check-language. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Sergei Mozhaev <mozhaev.dev@gmail.com>
9223e4f to
a76d588
Compare
- DELETE closure signals: by the write contract DELETE evidence rows carry an empty value, while the current reader filters non-empty values only — tombstones never reach the fold. Recorded as a known gap (seed component, seed process) with the reader fix scoped into the manual-resolution feature; the review queue's evidence read is normatively a per-account UPSERT/DELETE fold so closed accounts drop out (endpoint, ADR, decomposition). - GDPR purge cascade made physical: identity_inputs is a storage-less union view, so erasure targets the per-connector staging tables and delegates upstream history/raw erasure to the ingestion domain's purge hook (stated prerequisite; partial completion reported when unavailable); transformations consult the deny-list so rebuilds cannot re-materialize erased values. Deny-list pinned to a keyed digest: HMAC-SHA256 of the normalized value under a per-tenant key in the platform secret store, with key rotation/destruction semantics. - Precision: the journal's id rows do carry the account id in value_id; only attribute observations lack an account reference (ADR-0003, DESIGN 4.3). All artifacts pass cfs validate / validate-toc / check-language. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Sergei Mozhaev <mozhaev.dev@gmail.com>
… persons-only queue mention Versioned deny-list keys instead of rotate-in-place: erased plaintext cannot be re-digested, so each tombstone records its key version, new tombstones use the current version, and candidate values are checked under every retained version (old versions stay retained - a digest key without plaintext discloses nothing). Deliberately destroying a key version crypto-shreds its tombstones and explicitly forfeits their re-link protection: a re-delivered value surfaces as ordinary contested/pending evidence for review instead of silently linking. Also replaces the last persons-only review-queue phrasing (future-table summary row) with the normative active-evidence fold + current bindings definition. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Sergei Mozhaev <mozhaev.dev@gmail.com>
…ction Key versions with live tombstones MUST be retained for the deny-list to function; they are secrets (key plus digest permits verifying guessed values) and live under the same access control as the current key. Destroying a version is a separate, explicitly-confirmed administrative act that forfeits re-link protection entirely: a re-delivered copy of those values flows through ordinary automatic processing (it may auto-link or mint a person like any new evidence), and the destruction record in the operations journal must state this consequence. Drops the incorrect claims that such values would still surface for review and that a digest key without plaintext discloses nothing. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Sergei Mozhaev <mozhaev.dev@gmail.com>
- Seed-side timestamp collision documented: two accounts of one source resolving to one person at the same _synced_at would collide on their id rows under the account-less natural key; the seed carries the same per-account timestamp-uniqueness obligation as the correction path (DESIGN seed step + index note, ADR-0003 enabler 4). - PRD person-domain contract rewritten off the legacy aliases wording: the persons journal is authoritative, this domain mints person_id, the person domain is a reader. - Correction durability marked conditional on the seed hardening deployment (divergent-group gap named as the one violation path); acceptance criterion covers e-mail-less accounts via the evidence-derived queue. - Seed process fixes: e-mail map loading described as latest-wins with person_id; value routing lists the id-like and name-like types the implementation actually routes; no-delete rule scoped to persons with the cache rebuild allowed; seed-run inspection GET endpoints added to the implemented API table (no HTTP seed trigger exists). - ADR-0002 post-acceptance note extended: DATETIME(6) migrations, transactional cache rebuild vs RENAME sketch, implemented auto-link vs the quarantine of section 6; DESIGN wins on conflict. - DECOMPOSITION: persons entry is append-only; Feature 4 -> Feature 3 acceptance-surface dependency in overview and graph; excluded sentinel contract covers all consumers; duplicate proposals entry dropped from the late-phase list; legacy alias artifacts annotated. - markdownlint: language tags on ASCII-diagram fences, blockquote fix. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Sergei Mozhaev <mozhaev.dev@gmail.com>
What
Syncs
docs/domain/identity-resolution/with the implemented architecture and records the manual-resolution decision. The v2.0 spec described a ClickHouse-native alias pipeline (aliasesresolution store, BootstrapJob/MatchingEngine/ResolutionService,unmapped/conflicts/merge_audits) that was never built; the shipped path is the append-onlypersonsjournal.DESIGN.md → v3.0
identity_inputs(evidence) → persons-seed fold →persons(MariaDB, append-only, source of truth) → persons-sync mirror → dbtresolve_person_idat build time.created_at, case-insensitivevalue_id,DATETIME(6));aliasesmarked legacy; unbuilt tables compressed to future-table summaries (full DDL remains in git history).ADR-0003 (proposed)
Operator identity corrections as append-only
personsobservations — no separate decision store in v1; three considered options, required enablers, and explicit revisit triggers (auto-matcher, multi-operator, recurring re-decisions, value blocklists).PRD.md
cpt-ir-fr-bootstrap-incrementalunchecked (full-set fold today).DECOMPOSITION.md
Features rewritten around the shipped architecture (history note points to git history for the original plan); new entry 2.4 Manual Resolution; future matcher reframed as proposals-only with acceptance flowing through the operator API.
Registry
artifacts.toml: domain ADR-0002/0003 registered; ADR-0002 gains its missing**ID**line and a post-acceptance schema note.Validation
cfs validate,cfs validate-toc,cfs check-language, duplicate-ID check — PASS for DESIGN, PRD, DECOMPOSITION and ADR-0003.Known debt (out of scope here): ADR-0002 predates the artifact template and keeps its original section structure — accepted ADRs are not restructured; flagged for a separate cleanup if desired.
Context
feature-manual-resolution/FEATURE.md+ component-spec extension for the operator endpoints.🤖 Generated with Claude Code
Summary by CodeRabbit