docs(identity-resolution): respec working draft#1790
Draft
mitasovr wants to merge 1 commit into
Draft
Conversation
Thesis-style working draft of the identity-resolution redesign, collected from review-session statements. Covers the persons decision-log model, field statuses and canonical value selection, the resolver/proposals flow with optimistic concurrency, storage and replication, org chart, and rejected alternatives. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Signed-off-by: Roman Mitasov <Roman.Mitasov@constructor.tech>
📝 WalkthroughWalkthroughChangesIdentity Resolution Respec
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🤖 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/RESPEC-DRAFT.md`:
- Line 73: Fix the list indentation surrounding “Protection against duplicate
follow on a scalar kind” in the RESPEC draft so its nesting matches adjacent
list items and markdownlint passes.
- Line 215: Update the visibility-read description to remove the ClickHouse
range-dictionary requirement and describe the intended interval-join mechanism
for filtering rows by ancestor and interval at T; ensure the draft no longer
contradicts the dictionary-removal objective.
- Around line 21-23: Revise the persons model definition and the related §1.4
materialization description to distinguish decision records from mechanically
materialized source records. Define each row class and specify which
participates in canonical selection, history, and audit, while preserving the
intended semantics that only reviewed decisions represent truth.
- Around line 27-29: Resolve the contradiction in the reassignment description
and the related status definition: choose whether tombstones store the closed
value or null, then consistently update both sections. Explicitly define how
historical queries identify the closed assignment, and ensure the reassignment
writer and status semantics use the same representation.
- Around line 16-17: Update the canonical-selection rules in the narrow-row
shape and the corresponding sections around attribution and decision resolution
to define an explicit as-of timestamp cutoff, exclude assignments superseded by
later tombstones, and model each assignment’s logical validity interval. Specify
deterministic tie-breaking when records share the same timestamp, and replace
the “latest record is not a tombstone” shortcut with this interval- and
tombstone-aware rule.
- Around line 212-215: Update the org_chart_edges and org_chart_closure
specification to define interval boundary semantics: state whether valid
intervals are half-open or closed, how an open-ended valid_to is represented
when NULL, and the required timestamp precision and time zone. Ensure the
visibility-at-T rule and adjacent epoch behavior follow these definitions
without boundary ambiguity.
- Around line 135-136: Update the ClickHouse replication description in the
identity-resolution specification to replace the absolute “always holds a
current copy” claim with an explicit freshness contract. Define the permitted
replication lag, read-after-write behavior, and the fallback or backfill path
for attribution queries requiring newly accepted identity decisions, while
retaining the direct gold-level query join context.
- Around line 135-137: The identity-resolution specification must make tenant
isolation explicit for every table and execution path used by gold-level
queries, including persons and silver.class_git_commits. Update the §4/§7 design
and example query to define and test RLS policies for each queried table, or add
explicit tenant_id predicates wherever RLS is not guaranteed; preserve isolation
when identical emails exist across tenants.
- Around line 121-125: Define the atomic OCC protocol for every persons
mutation, including the transaction boundary and mechanism that checks the
client’s state identifier while applying the write. Specify the required
isolation, row/table locking, or CAS/version predicate so concurrent writers
cannot both succeed, and preserve the documented 409 rejection and
first-writer-wins behavior.
- Around line 104-115: Update the proposal acceptance flow to validate edited
proposals server-side instead of trusting client-provided persons_update. Before
applying persons_update, re-check tenant scope, authorization, proposal kind and
status, and source ownership; preferably recompute the write from a server-held
proposal record while preserving the existing acceptance behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2268558d-ea57-45a6-a4d9-cedd52aab151
📒 Files selected for processing (1)
docs/domain/identity-resolution/specs/RESPEC-DRAFT.md
Comment on lines
+16
to
+17
| - `date` — when this value was assigned to this field. | ||
| - The narrow-row shape allows reading the state of all fields as of any moment in time. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Make canonical selection explicitly as-of and tombstone-aware.
The read rule selects the latest fixed/follow record but does not apply an as-of cutoff or exclude decisions closed by later tombstones. The attribution note’s “latest record is not a tombstone” shortcut is also insufficient without defining the logical assignment interval and deterministic ordering for equal timestamps.
Also applies to: 57-61, 194-195
🧰 Tools
🪛 LanguageTool
[style] ~17-~17: This phrase is redundant. Consider writing “moment” or “time”.
Context: ...ading the state of all fields as of any moment in time. ### 1.1 Semantics: decision log, not ...
(MOMENT_IN_TIME)
🤖 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/RESPEC-DRAFT.md` around lines 16 - 17,
Update the canonical-selection rules in the narrow-row shape and the
corresponding sections around attribution and decision resolution to define an
explicit as-of timestamp cutoff, exclude assignments superseded by later
tombstones, and model each assignment’s logical validity interval. Specify
deterministic tie-breaking when records share the same timestamp, and replace
the “latest record is not a tombstone” shortcut with this interval- and
tombstone-aware rule.
Comment on lines
+21
to
+23
| - `persons` is the history of **decisions** to assign field values to a canonical profile, made on whatever criteria. | ||
| - There are no unreviewed records. If a record exists, it is truth. | ||
|
|
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Separate decision records from materialized records in the model.
persons is described as a decision log where every row is truth, but §1.4 also inserts status-less source-materialized rows that are explicitly mechanical consequences. Define the two row classes and clarify which one participates in canonical selection, history, and audit; otherwise consumers cannot interpret persons consistently.
Also applies to: 42-49
🤖 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/RESPEC-DRAFT.md` around lines 21 - 23,
Revise the persons model definition and the related §1.4 materialization
description to distinguish decision records from mechanically materialized
source records. Define each row class and specify which participates in
canonical selection, history, and audit, while preserving the intended semantics
that only reviewed decisions represent truth.
Comment on lines
+27
to
+29
| - Reassign (profile turns out to belong to someone else): two rows — | ||
| - tombstone at the old person: `{person_id: p1, ..., value: null, date: D2}`; | ||
| - assignment at the new person: `{person_id: p2, ..., value: gl1, date: D2}`. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Resolve the contradictory tombstone representation.
Reassignment writes a tombstone with value: null, while the status definition requires the tombstone to carry the closed value and never be null. Choose one representation and define how the closed assignment is identified; otherwise writers and historical queries will implement incompatible semantics.
Also applies to: 44-49
🤖 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/RESPEC-DRAFT.md` around lines 27 - 29,
Resolve the contradiction in the reassignment description and the related status
definition: choose whether tombstones store the closed value or null, then
consistently update both sections. Explicitly define how historical queries
identify the closed assignment, and ensure the reassignment writer and status
semantics use the same representation.
| | bamboohr | display_name | Anna Ivanova-Petrova | — | follower | 07-15 | | ||
|
|
||
| - Canonical today: the 04-03 decision points at the bamboohr stream → its latest record → `Anna Ivanova-Petrova`. | ||
| - Protection against duplicate follow on a scalar kind: |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the list indentation.
markdownlint reports inconsistent indentation here; adjust the surrounding list nesting so this item renders at the intended level.
🧰 Tools
🪛 markdownlint-cli2 (0.23.0)
[warning] 73-73: Inconsistent indentation for list items at the same level
Expected: 2; Actual: 0
(MD005, list-indent)
🤖 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/RESPEC-DRAFT.md` at line 73, Fix the
list indentation surrounding “Protection against duplicate follow on a scalar
kind” in the RESPEC draft so its nesting matches adjacent list items and
markdownlint passes.
Source: Linters/SAST tools
Comment on lines
+104
to
+115
| - Proposal shape (approximate): | ||
| - `id` — uuid. | ||
| - `unique_key` — stable unique key, built so that identical proposals from different resolvers merge into one. | ||
| - `kind` — proposal type; determines how the proposal is rendered. | ||
| - `inputs` — array of `identity_inputs` objects the conclusion was based on. | ||
| - `persons_update` — array of objects to be inserted into `persons` on acceptance. | ||
| - `confidence` — 0..1. | ||
| - `confidence` semantics: | ||
| - Determines position in the proposals list (higher — closer to the top). | ||
| - `confidence = 1` — can be accepted all at once (bulk accept). | ||
| - Below 1 — accepted one by one. | ||
| - Any proposal can be edited before acceptance. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n## File outline\n'
ast-grep outline docs/domain/identity-resolution/specs/RESPEC-DRAFT.md --view expanded || true
printf '\n## Relevant lines 80-140\n'
sed -n '80,140p' docs/domain/identity-resolution/specs/RESPEC-DRAFT.md | cat -n
printf '\n## Search proposal-related terms\n'
rg -n "proposal|persons_update|acceptance|bulk accept|edit(ed)? before acceptance|source-ownership|tenant|authorization|recompute" docs domain . -g '!**/node_modules/**' -g '!**/.git/**' || trueRepository: constructorfabric/insight
Length of output: 50382
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## Files mentioning RESPEC-DRAFT or proposals"
git ls-files | rg 'RESPEC-DRAFT\.md|proposal|identity-resolution' || true
echo
echo "## Search within docs for validation / recompute wording"
rg -n "recompute|server-side|authorization|tenant|source ownership|ownership|validate|validation|accept.*proposal|edit.*proposal|persons_update|bulk accept" docs -g '*.md' || trueRepository: constructorfabric/insight
Length of output: 50382
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## RESPEC-DRAFT around proposal editing"
sed -n '90,130p' docs/domain/identity-resolution/specs/RESPEC-DRAFT.md | cat -n
echo
echo "## Search within the identity-resolution domain for proposal acceptance/editing rules"
rg -n "proposal|persons_update|accept|edit before acceptance|bulk accept|source ownership|tenant|authorization|validate|recompute" docs/domain/identity-resolution -g '*.md' || trueRepository: constructorfabric/insight
Length of output: 29393
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '130,180p' docs/domain/identity-resolution/specs/RESPEC-DRAFT.md | cat -n
echo
rg -n "accept|proposal.*edit|persons_update|status|RLS|authorization|tenant" docs/domain/identity-resolution/specs/RESPEC-DRAFT.md docs/domain/identity-resolution/specs/PRD.md docs/domain/identity-resolution/specs/DESIGN.md || trueRepository: constructorfabric/insight
Length of output: 28166
Validate edited proposals server-side. persons_update should not be accepted verbatim from the client; on acceptance, re-check tenant, authorization, kind/status, and source ownership, or recompute the write from a server-held proposal record.
🤖 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/RESPEC-DRAFT.md` around lines 104 -
115, Update the proposal acceptance flow to validate edited proposals
server-side instead of trusting client-provided persons_update. Before applying
persons_update, re-check tenant scope, authorization, proposal kind and status,
and source ownership; preferably recompute the write from a server-held proposal
record while preserving the existing acceptance behavior.
Comment on lines
+121
to
+125
| - State identifier = `max(persons.id)` (auto-increment) of the state the client read; exposed to clients as an opaque string with no guarantees about its content. | ||
| - Every `persons` mutation — proposal acceptance, manual link/unlink, edit — carries the state identifier received at read time (`GET /proposals`, profile read). | ||
| - On mismatch the server rejects with `409`; the client refetches and reconsiders on the fresh state. First writer wins. | ||
| - Network retries are deduplicated via an explicit `Idempotency-Key`, never by content comparison. | ||
| - Granularity starts global — any write invalidates every outstanding state identifier. May narrow to per-person later without changing the API contract. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Show the relevant section with line numbers
sed -n '100,150p' docs/domain/identity-resolution/specs/RESPEC-DRAFT.md | cat -n
# Find other mentions of the state identifier / OCC behavior
rg -n "state identifier|max\\(persons\\.id\\)|Idempotency-Key|409|first writer wins|compare-and-swap|CAS|optimistic concurrency|transaction isolation|locking|atomic" docs/domain/identity-resolution/specs/RESPEC-DRAFT.md docs/domain/identity-resolution -SRepository: constructorfabric/insight
Length of output: 12836
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the sections around idempotency / concurrency in the related design docs
sed -n '430,470p' docs/domain/identity-resolution/specs/PRD.md | cat -n
printf '\n---\n'
sed -n '1000,1045p' docs/domain/identity-resolution/specs/DESIGN.md | cat -n
printf '\n---\n'
sed -n '1388,1415p' docs/domain/identity-resolution/specs/DESIGN.md | cat -n
# Find any direct mention of persons mutations or state tokens outside the draft
rg -n "persons mutation|proposal acceptance|manual link|manual unlink|profile read|state identifier|max\\(persons\\.id\\)|first writer wins|409" docs/domain/identity-resolution/specs -SRepository: constructorfabric/insight
Length of output: 7978
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search for any explicit write protocol for persons or comparable versioning terms
rg -n "SELECT .*FOR UPDATE|FOR UPDATE|SERIALIZABLE|REPEATABLE READ|READ COMMITTED|version column|optimistic|compare-and-swap|CAS|updated_at|row version|etag|person.*version|persons.*version|persons.*FOR UPDATE|state token|conflict" docs/domain/identity-resolution/specs -S
# Inspect nearby sections that might define persons mutation behavior
rg -n "person(s)? mutation|link/unlink|edit|acceptance|profile read|GET /proposals|persons/" docs/domain/identity-resolution/specs/PRD.md docs/domain/identity-resolution/specs/DESIGN.md docs/domain/identity-resolution/specs/RESPEC-DRAFT.md -SRepository: constructorfabric/insight
Length of output: 18677
Define the atomic OCC check-and-write protocol.
max(persons.id) is only safe if the read, validation, and write happen in one transaction or via a CAS-style update. Otherwise two writers can read the same token, both pass the check, and both commit despite “first writer wins.” Specify the transaction isolation, locking, or versioning mechanism used by persons mutations.
🤖 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/RESPEC-DRAFT.md` around lines 121 -
125, Define the atomic OCC protocol for every persons mutation, including the
transaction boundary and mechanism that checks the client’s state identifier
while applying the write. Specify the required isolation, row/table locking, or
CAS/version predicate so concurrent writers cannot both succeed, and preserve
the documented 409 rejection and first-writer-wins behavior.
Comment on lines
+135
to
+136
| - `persons` lives in MariaDB; it is continuously replicated to ClickHouse (mechanism secondary — Airbyte or similar), so ClickHouse always holds a current copy. | ||
| - The ClickHouse copy is what gold-level queries join against directly. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== target file context =="
nl -ba docs/domain/identity-resolution/specs/RESPEC-DRAFT.md | sed -n '115,155p'
echo
echo "== related mentions =="
rg -n "freshness|lag|read-after-write|backfill|fallback|Airbyte|ClickHouse|gold-level queries|continuously replicated|current copy" docs/domain/identity-resolution -SRepository: constructorfabric/insight
Length of output: 229
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== target file context =="
awk 'NR>=115 && NR<=155 { printf "%d:%s\n", NR, $0 }' docs/domain/identity-resolution/specs/RESPEC-DRAFT.md
echo
echo "== related mentions =="
rg -n "freshness|lag|read-after-write|backfill|fallback|Airbyte|ClickHouse|gold-level queries|continuously replicated|current copy" docs/domain/identity-resolution -SRepository: constructorfabric/insight
Length of output: 22735
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== repo files for identity-resolution docs =="
git ls-files 'docs/domain/identity-resolution/**' | sed -n '1,200p'Repository: constructorfabric/insight
Length of output: 596
Replace “always holds a current copy” with a freshness contract. Continuous replication alone does not define max lag, read-after-write behavior, or the fallback/backfill path for attribution queries that need newly accepted identity decisions.
🤖 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/RESPEC-DRAFT.md` around lines 135 -
136, Update the ClickHouse replication description in the identity-resolution
specification to replace the absolute “always holds a current copy” claim with
an explicit freshness contract. Define the permitted replication lag,
read-after-write behavior, and the fallback or backfill path for attribution
queries requiring newly accepted identity decisions, while retaining the direct
gold-level query join context.
Comment on lines
+135
to
+137
| - `persons` lives in MariaDB; it is continuously replicated to ClickHouse (mechanism secondary — Airbyte or similar), so ClickHouse always holds a current copy. | ||
| - The ClickHouse copy is what gold-level queries join against directly. | ||
| - Tenant scoping is enforced via RLS; queries do not carry explicit tenant conditions. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="docs/domain/identity-resolution/specs/RESPEC-DRAFT.md"
echo "== File info =="
wc -l "$file"
echo
echo "== Relevant excerpts (lines 120-220) =="
nl -ba "$file" | sed -n '120,220p'
echo
echo "== Search for tenant/RLS/policy references in the spec =="
rg -n "RLS|tenant|tenant_id|policy|row-level|row level|ClickHouse|MariaDB|silver\.class_git_commits|persons" "$file"Repository: constructorfabric/insight
Length of output: 319
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="docs/domain/identity-resolution/specs/RESPEC-DRAFT.md"
echo "== Excerpts with line numbers =="
awk 'NR>=120 && NR<=220 { printf "%4d %s\n", NR, $0 }' "$file"
echo
echo "== Exact tenant/RLS-related mentions =="
grep -nE 'RLS|tenant|tenant_id|policy|row-level|row level|ClickHouse|MariaDB|silver\.class_git_commits|persons' "$file" || trueRepository: constructorfabric/insight
Length of output: 10672
🏁 Script executed:
#!/bin/bash
set -euo pipefail
file="docs/domain/identity-resolution/specs/RESPEC-DRAFT.md"
echo "== Excerpts with line numbers (220-243) =="
awk 'NR>=220 && NR<=243 { printf "%4d %s\n", NR, $0 }' "$file"
echo
echo "== Search for any concrete RLS/policy definitions outside the main sections =="
grep -nE 'tenant_id|RLS|policy|row-level|row level|grants|filtering by tenant|execution path|class_git_commits|commits' "$file" || trueRepository: constructorfabric/insight
Length of output: 4335
Make tenant isolation explicit on every table touched by the query. §4 says RLS is enforced, but §7 still leaves RLS/tenant_id filtering unresolved. The example query reads from both persons and silver.class_git_commits; if only persons is covered, identical emails can still pull commit rows across tenants. Define and test tenant policies for every queried table and execution path, or add explicit tenant predicates where the policy is not guaranteed.
🤖 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/RESPEC-DRAFT.md` around lines 135 -
137, The identity-resolution specification must make tenant isolation explicit
for every table and execution path used by gold-level queries, including persons
and silver.class_git_commits. Update the §4/§7 design and example query to
define and test RLS policies for each queried table, or add explicit tenant_id
predicates wherever RLS is not guaranteed; preserve isolation when identical
emails exist across tenants.
Comment on lines
+212
to
+215
| - `org_chart_edges` — source of truth; an edge is a decision record: `(person, supervisor, org_chart_id, valid_from, valid_to)`. | ||
| - `org_chart_closure` — derived ancestor→descendant pairs; the pair's interval = intersection of the intervals of the path's edges. Rebuilt from edges; never a source of truth. | ||
| - One row per epoch of a relation. Row count grows with the number of *changes*, not with time. | ||
| - Visibility read at moment `T`: filter closure by `ancestor = viewer` and interval containing `T`. The same rows serve any date; in ClickHouse — a range dictionary probed per fact row. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Specify interval boundary semantics.
Define whether intervals are half-open or closed, how valid_to = NULL is represented, and the timestamp precision/time zone. Without this, adjacent org-chart epochs can overlap or produce boundary-time ambiguity in visibility and attribution.
🤖 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/RESPEC-DRAFT.md` around lines 212 -
215, Update the org_chart_edges and org_chart_closure specification to define
interval boundary semantics: state whether valid intervals are half-open or
closed, how an open-ended valid_to is represented when NULL, and the required
timestamp precision and time zone. Ensure the visibility-at-T rule and adjacent
epoch behavior follow these definitions without boundary ambiguity.
| - `org_chart_edges` — source of truth; an edge is a decision record: `(person, supervisor, org_chart_id, valid_from, valid_to)`. | ||
| - `org_chart_closure` — derived ancestor→descendant pairs; the pair's interval = intersection of the intervals of the path's edges. Rebuilt from edges; never a source of truth. | ||
| - One row per epoch of a relation. Row count grows with the number of *changes*, not with time. | ||
| - Visibility read at moment `T`: filter closure by `ancestor = viewer` and interval containing `T`. The same rows serve any date; in ClickHouse — a range dictionary probed per fact row. |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Remove the stale ClickHouse Dictionary requirement.
The PR objective says the ClickHouse Dictionary is being removed, but this line still requires a range dictionary for every fact row. Replace it with the intended interval-join mechanism or explicitly reconcile the objective and draft.
🤖 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/RESPEC-DRAFT.md` at line 215, Update
the visibility-read description to remove the ClickHouse range-dictionary
requirement and describe the intended interval-join mechanism for filtering rows
by ancestor and interval at T; ensure the draft no longer contradicts the
dictionary-removal objective.
mitasovr
marked this pull request as draft
July 16, 2026 08:43
mitasovr
requested review from
aleksdotbar,
cyberantonz,
dzarlax,
ktursunov and
mozhaev-dev
July 16, 2026 08:43
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Adds
docs/domain/identity-resolution/specs/RESPEC-DRAFT.md— a thesis-style working draft of the identity-resolution redesign, collected from a spec review session. It is not yet reconciled with the existing PRD/DESIGN/ADRs; it records the target principles the rewrite will be based on.Divergence from the current specs
Retained (compatible)
identity_inputsas the connector-facing observation intake: schema,identity_inputs_from_historydbt macro, incremental processing.personsin MariaDB; the identity service owns the schema and is the single writer (ADR-0006); the MariaDB-transactional / ClickHouse-analytical storage split.person_id= random UUIDv7, stable, never re-derived from mutable attributes (core of ADR-0002).Contradicts the current specs
personssemantics: "observation history" (PRDcpt-ir-fr-persons-history, DESIGN §3.7) → a journal of decisions; no unreviewed records — a record is truth.reason='pending-iresolution'rows (ADR-0002 §6) are disallowed.>= 1.0 → auto-link, cold-path auto-create, the ">= 80% auto-resolved in 30 min" goal) is replaced by proposals accepted only by the user. Confidence now orders proposals and gates bulk-accept; it never triggers writes.aliases, ResolutionService, and the CH Dictionary are dropped. Gold queries join a ClickHouse replica ofpersonsdirectly; resolution is a documented SQL algorithm, not an API hot path.match_rules,unmapped,conflictstables go with them: the operator queue becomes a computedGET /proposals(dedup/hide viaunique_key); conflicts become sanitary resolvers.merge_auditssnapshots → reassignment as ordinary decision records (tombstone + assign); the journal is its own audit trail. Optimistic concurrency (state_version=max(persons.id)as an opaque token, 409 on staleness,Idempotency-Keyfor retries) replaces snapshot-based atomicity.statusrecords (follow/fixed/tombstone) written only on decisions; the person-domain golden-record spec is absorbed.person_idminting moment: at seed-time first observation → at proposal acceptance.personsreplica and on all joined fact tables; MariaDB has no native RLS — isolation stays in the single-writer service).New (no counterpart in the current specs)
GET /proposals,unique_keydedup + hidden list, edit-before-accept, bulk-accept at confidence 1.org_chart_edges(decision records, SCD2 intervals) + derivedorg_chart_closure; forest is normal, strict gap semantics, acyclicity as the only write-time invariant, integrity resolver, chart switching via dry-run.person_ideverywhere email is sent today.Known gaps (old specs cover it, the draft does not yet)
personsjournal.account_person_map.GET /proposalslatency,persons→ ClickHouse replica lag).POST /v1/profiles, ADR-0003/0004/0007).🤖 Generated with Claude Code
Summary by CodeRabbit