feat(people): add reversible profile merges

[salmonumbrella]

What changed

• Add atomic person-profile merges on SQLite and PostgreSQL. The selected survivor keeps its person ID and vCard UID, the absorbed UID becomes an alias, participant bindings and curated rows are reconciled, and the full operation is recorded in a durable snapshot and row journal.
• Add lineage-aware splits into a fresh person and UID. Exact reversal restores the pre-merge profiles, partial splits move participant-attributable data, and accepted merge-review decisions prevent a false exact reversal.
• Expose merge, split, history, snapshot inspection, and conflicting-attribute decisions through the API, CLI, OpenAPI contract, and generated Go and web clients. Identity-link conflicts now return both profiles for an explicit merge instead of leaving a generic binding conflict.
• Preserve merge history through backups and complete subset exports, and keep activity, tracking, contact state, cache state, and person/identity revisions consistent across merge, split, and idempotent replay.

Why

Two profiles for the same human could not be joined once both had been promoted without deleting one side and losing curated data. There was also no reliable way to undo a mistaken join. This adds a human-controlled, auditable path in both directions without reusing IDs or published UIDs.

Usage

msgvault person merge 123 456 \
  --survivor-revision 7 \
  --absorbed-revision 4 \
  --idempotency-key merge-123-456

msgvault person merge-history 123
msgvault person merge-show 42

msgvault person split 123 \
  --merge-id 42 \
  --participant 789 \
  --revision 8 \
  --idempotency-key split-42-789

Repeat --participant when splitting more than one absorbed lineage.

Closes #638

[roborev-ci]

roborev: Combined Review (8c411be)

Code review found one high-severity privacy issue and three medium-severity data-integrity issues.

High

• internal/store/subset.go:928 — Merge snapshots leak activity outside the subset boundary. copyPersonMergePackets copies person_merges.snapshot_blob wholesale, exposing excluded message IDs, interaction metadata, account-owner identifiers, and contact-history aggregates. Decode and scope snapshots before copying: remove out-of-scope derived activity/contact-state rows or reject invalid packets, then recompute the snapshot hash.

Medium

• internal/store/person_splits.go:786 — Exact reversal overwrites post-merge person-reference edits. Restore references only when their current value still matches the recorded post-merge value, accounting for chained-merge rebasing.

• internal/store/person_merges.go:1478 — Relationship-review deduplication omits source_resource_uid. This collapses distinct RELATED occurrences and loses decisions and provenance. Include source_resource_uid in the duplicate predicate.

• internal/store/person_merges.go:1566 — Deleting duplicate employment leaves native vCard mappings dangling. Rebind suitable mappings to the surviving employment or release them to residue before deletion while preserving split restoration.

---

Reviewers: 2 done | Synthesis: codex, 11s | Total: 14m32s

[roborev-ci]

roborev: Combined Review (1d9c960)

Verdict: Changes requested — one high-severity data leak and one medium-severity reversal bug.

High

• Merge snapshots can leak data excluded from shared subsets — internal/store/subset.go:1105
  Automatically copied split, recomputed, and derived merge-journal snapshots may contain relationships, notes, dates, or metadata excluded from the subset. Validate each snapshot row and its ownership closure against the destination; omit or sanitize the merge packet if embedded data falls outside the export boundary.

Medium

• Exact reversal leaves duplicate source support — internal/store/person_splits.go:489
  Reversal restores the absorbed candidate’s source-support row but leaves a merge-created copy on the survivor. Track whether the destination support key existed before the merge, remove it during reversal only when the merge created it, and add coverage for colliding candidates backed by distinct sources.

---

Reviewers: 2 done | Synthesis: codex, 9s | Total: 12m24s

[roborev-ci]

roborev: Combined Review (d4e68cc)

One-line verdict: Two medium-severity issues remain: relationship-review deduplication can lose provenance, and subset exports can leak out-of-scope activity data.

Medium

• internal/store/person_merges.go:1470 — Deduplication collapses distinct relationship reviews. The duplicate predicate ignores source_resource_uid, so otherwise-identical RELATED occurrences from different vCard resources may be merged, discarding independent review decisions and provenance. Include source_resource_uid in the predicate and test reviews from distinct resources.

• internal/store/subset.go:928 — Merge snapshots leak activity outside the exported subset. copyPersonMergePackets copies snapshot_blob unchanged, including all activity_event_persons and person_contact_state rows for both people rather than filtering by selected_messages. This can expose out-of-scope message IDs, dates, interaction data, contact timestamps, and owner addresses. Strip derived activity/contact-state rows and recompute hashes/journal paths, or omit merge packets containing activity outside the selected subset.

---

Reviewers: 2 done | Synthesis: codex, 9s | Total: 15m47s

[roborev-ci]

roborev: Combined Review (86b7d0d)

One high-severity security issue and two medium-severity data-integrity issues were found.

High

• internal/store/subset.go:1130 — Split journal rows can leak excluded data. personMergePacketRowsComplete treats every row with split_id as complete without verifying that its data remains within the subset boundary. Merge snapshots and post_merge_row_json can therefore expose excluded normalized identifiers, notes, and evidence. Require current rows and referenced endpoints to exist in the destination, or omit the entire merge packet when its dependency closure cannot be proven in scope.

Medium

• internal/store/person_splits.go:716 — Exact splits overwrite post-merge reassignments. Person-reference columns are restored before the three-way comparison, potentially moving an employment back into the recreated profile and leaving the former owner’s projection stale. Restore references only when their current value still matches the rebased post-merge owner; otherwise preserve the reassignment and update affected projections.

• internal/store/person_merges.go:1567 — Duplicate employment deletion can remove the sole primary. If the absorbed duplicate is primary and the surviving duplicate is not, deletion can leave the merged person without a primary employment or derived organization/title projection. Promote the survivor when necessary and journal that mutation for reversal.

---

Reviewers: 2 done | Synthesis: codex, 10s | Total: 12m42s

[roborev-ci]

roborev: Combined Review (ed6ec8b)

One medium-severity issue affects scoped subset exports.

Medium

• Merge snapshots can leak records excluded from scoped exports — internal/store/subset.go:1031, internal/store/subset.go:1047
personMergePacketRowsComplete validates derived snapshot rows and surviving person_merge_rows, but unchanged survivor-side rows are pruned from that journal while remaining in snapshot_blob. The exported packet can therefore expose excluded relationships, identity candidates, normalized values, source references, notes, or evidence. Validate every row and dependency in snapshot.Rows against the destination’s selected-data closure, and omit the entire packet if any content would otherwise be excluded or redacted.

• Splitting later merges can leave earlier merge journals stale — internal/store/person_splits.go:745
  Split restoration can change composite keys without rebasing earlier unsplit journals. Reversing an earlier merge may then treat an existing row as deleted and leave it attached to the wrong person. Atomically rebase affected earlier journals’ current_row_id, current_row_key, and disposition whenever a split changes, recreates, or removes a journaled row.

---

Reviewers: 2 done | Synthesis: codex, 13s | Total: 17m19s

[roborev-ci]

roborev: Combined Review (cd29064)

Changes need fixes for two medium-severity data-integrity issues.

Medium

• internal/store/person_splits.go:480 — Exact splits unconditionally recreate rows marked deleted_snapshot. Duplicate employment and tracking rows lack a surviving-row locator, so deleting the surviving merged state before splitting can resurrect the absorbed copy.

  • Fix: Journal the deduplication target and restore the absorbed row only if that target still exists; otherwise preserve the post-merge deletion. Add coverage for deleting deduplicated employment or tracking state before splitting.

• internal/store/subset.go:1371 — Merge-snapshot validation checks service_id using the raw source ID, although communication-service IDs are database-local and remapped during subset export. If that ID maps to a different destination service, the packet remains and a later split can restore contact data against the wrong service.

  • Fix: Validate through selected_profile_service_map and omit packets whose snapshot IDs require remapping, or consistently rewrite and re-checksum the snapshot and journal data.

---

Reviewers: 2 done | Synthesis: codex, 12s | Total: 14m56s

[roborev-ci]

roborev: Combined Review (03ade92)

Changes need correction before merging due to a split-path failure involving recomputed activity rows.

Medium

• internal/store/person_splits.go:525 — Participant-exact activity rows may have action = "recomputed" without a current row key when a merge removes the activity link, such as when the absorbed participant becomes an alias of an owner person. The split path still attempts to restore that row and fails before activity recomputation runs.
  • Fix: Treat action == "recomputed" rows as derived in both restoration loops: mark their journal entries as split and let recomputePersonSplitActivityTx rebuild them from participant evidence. Add coverage for an absorbed activity link that disappears during merge.

---

Reviewers: 2 done | Synthesis: codex, 12s | Total: 22m16s

[roborev-ci]

roborev: Combined Review (c205259)

Merge/split logic has two medium-severity data integrity issues that can corrupt subset exports or lose post-merge tracking state.

Medium

• Invalid merge journal key for tracking rows — internal/store/person_merges.go:1211
  Merge journal dispositions always encode current_row_key with the column name id, but person_tracking uses person_id. Subset export can reject an otherwise valid merge packet as corrupt when validating this key against the table registry.
  Fix: Build the key from the table’s registered key columns and add a subset-export test for a merge where only the absorbed person is tracked.

• Exact split can overwrite newly recreated tracking state — internal/store/person_splits.go:818
  After an absorbed-only tracking row is merged, untracked, and recreated, exact split treats the reused key as the original row and moves the new tracking row to the split person. This loses the source person’s post-merge tracking state.
  Fix: Compare the current row with the journaled post-merge row to detect delete-and-recreate replacements. Preserve replacements on the source while recreating the original snapshot for the split person, and test the merge → untrack → re-track → split sequence.

---

Reviewers: 2 done | Synthesis: codex, 14s | Total: 25m18s

[roborev-ci]

roborev: Combined Review (bef2dbe)

The person split exactness logic has one medium-severity correctness issue; no security issues were identified.

Medium

• internal/store/person_splits.go:400 — Exactness only considers absorbed-origin lineage. After splitting an earlier nested merge, survivor-origin participants from a later merge may belong to another person, but reversing that later merge is still classified as exact. Restoration then maps survivor snapshot references to sourceID, potentially moving rebased rows away from their current owner and corrupting profile ownership.
  • Fix: Require all survivor-origin lineage to remain bound to the source before declaring an exact reversal. Otherwise, perform a partial split or use owner-aware restoration. Add coverage for splitting an earlier merge before reversing a later merge.

---

Reviewers: 2 done | Synthesis: codex, 12s | Total: 15m49s

[roborev-ci]

roborev: Combined Review (fa1192b)

Medium-severity lineage handling issue found; no security vulnerabilities identified.

Medium

• internal/store/person_merges.go:1838 — A later participant merge can collapse mixed survivor/absorbed provenance to "absorbed". This may cause validatePersonSplitLineage to misclassify a split as an exact reversal, restoring all absorbed profile data and retargeting the retired UID even though survivor lineage is also moved.
  • Fix: Preserve mixed/dual provenance during participant consolidation, reject exact reversal whenever a selected participant contains survivor provenance, and add coverage for cross-origin consolidation when the survivor has multiple participants.

---

Reviewers: 2 done | Synthesis: codex, 10s | Total: 13m49s

[roborev-ci]

roborev: Combined Review (ee6e447)

Code review found one medium-severity correctness issue in exact-split dependency handling; no security vulnerabilities were identified.

Medium

• internal/store/person_splits.go:119 — The exact-split guard checks accepted candidates only for the merge being split. If a pending candidate from an older merge is retargeted through this merge and later accepted, splitting moves the candidate back to the new person while its new resolution_value_id remains owned by the source person, breaking candidate/value ownership.
  • Suggested fix: Include absorbed-origin candidate rows from the merge journal in the reviewed-dependency check, or move/rebase their post-merge resolution values during the split.

---

Reviewers: 2 done | Synthesis: codex, 9s | Total: 22m9s

[roborev-ci]

roborev: Combined Review (728a339)

Changes look sound overall, with one medium-severity concurrency issue to address.

Medium

• internal/api/person_merges.go:292 — The candidate decision commits before the handler reloads the person to generate its ETag. A concurrent merge can absorb that person between these operations, causing a 404 even though the decision succeeded; retrying may then conflict because the candidate was retargeted. Return the post-decision person revision from the store transaction and construct the response ETag from that atomic result.

---

Reviewers: 2 done | Synthesis: codex, 9s | Total: 19m0s

[roborev-ci]

roborev: Combined Review (3a15041)

The merge/split implementation is solid overall, but one medium-severity data-restoration issue should be fixed before merging.

Medium

• Person-reference columns bypass three-way restoration — internal/store/person_splits.go:853

  If a merged employment is later reassigned to a third person, splitting unconditionally remaps its original absorbed-person ID, overwriting the legitimate post-merge edit.

  Fix: Restore a remapped reference only when its current value still matches the recorded post-merge value; otherwise preserve the current value. Add a behavioral test covering reassignment before splitting.

---

Reviewers: 2 done | Synthesis: codex, 10s | Total: 29m35s

[roborev-ci]

roborev: Combined Review (9635e77)

The change needs a medium-severity fix for chained merge reversal correctness.

Medium

• internal/store/person_splits.go:787 — The three-way restore treats any person-reference change since the original merge as a user reassignment. If that merge’s survivor is later absorbed into another person, ordinary rows move again, but older journals are rebased only for deduplication/disposition paths. Directly reversing the earlier merge can therefore report an exact reversal while leaving absorbed-origin profile rows, employments, and envelopes attached to the later survivor.

  Suggested fix: Rebase older unsplit journals’ post_merge_row_json whenever a subsequent merge moves or repoints their current rows. Add coverage for a chained merge followed by direct reversal using a non-deduplicated profile or employment row.

---

Reviewers: 2 done | Synthesis: codex, 10s | Total: 12m23s

[roborev-ci]

roborev: Combined Review (c588502)

Code is generally sound, but subset exports have one medium-severity identity revision consistency issue.

Medium

• internal/store/subset.go:934 — Copying merge and split packets preserves the source archive’s identity_revision values and serialized results, while the subset’s archive_metadata identity revision remains at its fresh default. Replaying a copied idempotency key can report a revision inconsistent with the destination’s cache authority, potentially causing clients to miss later mutations with lower revision numbers.
  • Fix: Seed the destination identity revision from the source when copying packets, or rebase both operation columns and serialized result revisions to the destination’s current revision.

---

Reviewers: 2 done | Synthesis: codex, 12s | Total: 12m46s

[roborev-ci]

roborev: Combined Review (f204759)

The change is not ready to merge: two medium-severity data-integrity issues remain in exact person splitting.

Medium

• Incorrect canonical UID on survivor-owned vCard envelopes — internal/store/person_splits.go:913

  Exact splitting assigns newUID to canonical_person_uid for every vCard envelope. Envelopes originally owned by the survivor retain the source person_id but incorrectly receive the split person’s UID, breaking ownership metadata and UID-based lookup/export.

  Fix: Derive the canonical UID from each envelope’s original owner: use the new UID for absorbed-origin envelopes and the current source UID for survivor-origin envelopes. Add coverage for envelopes on both merge participants.

• Concurrent supersede can be undone by an exact split — internal/store/person_merge_snapshot.go:292, internal/store/person_merges.go:836

  Inbound person-reference attributes are snapshotted without row locks, and PostgreSQL supersede operations do not acquire the identity lock. A supersede committed between the snapshot and bulk reference update can enter the recorded post-merge state; an exact split may then treat its active_until or superseded_at changes as merge-owned and reopen the value.

  Fix: Lock affected attribute rows before snapshotting, or serialize removal and superseding of existing record references with the identity mutation lock. Add a PostgreSQL interleaving test verifying that a concurrent supersede remains effective after an exact split.

---

Reviewers: 2 done | Synthesis: codex, 15s | Total: 16m9s

[roborev-ci]

roborev: Combined Review (d4f0ad0)

Code review found one medium-severity issue in merge/split reversibility.

Medium

• Incorrect source restoration after an exact split — internal/store/person_merges.go:1831
  Candidate source rows are always journaled as deduplicated when an absorbed candidate collapses into a survivor. If the survivor did not previously have that source, the merge creates a target row; an exact split then recreates the absorbed row but retains the merge-created survivor row, leaving both candidates supported by the same source.

  Fix: Track whether the target source row existed before reconciliation. Journal newly created targets as key moves and restore their candidate_id during splitting; reserve deduplication for preexisting targets. Add coverage for candidates with distinct source support.

---

Reviewers: 2 done | Synthesis: codex, 10s | Total: 28m35s

[roborev-ci]

roborev: Combined Review (86ad768)

Code review found one medium-severity correctness issue; no critical or high-severity findings.

Medium

• internal/store/person_splits.go:523 — Exact split can fail after snapshot dependencies are deleted. Rows marked deleted_snapshot are recreated without verifying that referenced dependencies still exist. For example, a merge may delete a custom-relationship self-edge; if its now-unused relationship type is subsequently deleted, splitting the merge attempts to restore the edge with the missing type ID, causing an internal database error and transaction rollback. Similar failures may affect identity-match support rows after source removal.
  • Fix: Validate external dependencies before restoring snapshot rows. If a dependency was removed after the merge, keep the row deleted; alternatively, prevent deletion of dependencies required by an unsplit merge snapshot. Add coverage for exact splits after deleting a custom relationship type or source.

---

Reviewers: 2 done | Synthesis: codex, 11s | Total: 9m57s

[roborev-ci]

roborev: Combined Review (ecc4c63)

The change is broadly sound, but one medium-severity split failure remains.

Medium

• internal/store/person_splits.go:854 — If a merge deletes a relationship referenced by an accepted review and that relationship’s type is later deleted, splitting skips recreating the relationship but still restores the review’s original accepted_relationship_id. The dangling reference violates the foreign-key constraint and causes the entire split to fail.
  • Fix: Track skipped parent relationships and retain the review’s post-merge NULL reference when its original relationship cannot be restored. Add coverage for an accepted review followed by post-merge deletion of the relationship type.

---

Reviewers: 2 done | Synthesis: codex, 11s | Total: 14m20s

[roborev-ci]

roborev: Combined Review (26ff4b8)

The merge/split implementation is generally sound, but two medium-severity correctness and scalability issues should be addressed.

Medium

• Deduplicated rows may be incorrectly restored after reassignment — internal/store/person_splits.go:523

  Exact reversal restores a deduplicated row whenever its retained counterpart exists, without checking whether that counterpart was reassigned after the merge. This can recreate an absorbed row while leaving the retained row with its new owner, undoing legitimate post-merge changes.

  Fix: Apply the ownership/three-way reconciliation used for moved rows: skip restoration if the retained row was reassigned, preserve supported post-merge field changes when reconstructing duplicates, and test deduplication followed by reassignment and exact splitting.

• Large activity histories can exceed SQLite’s parameter limit — internal/store/activity.go:2287, internal/store/person_splits.go:1701

  Merge and split reconciliation put every affected message ID into single IN queries for row locking and candidate loading. People with more than SQLite’s 32,766-parameter limit cannot be merged or split.

  Fix: Process sorted message IDs in bounded chunks for both locking and candidate loading while preserving deterministic ordering across chunks.

---

Reviewers: 2 done | Synthesis: codex, 11s | Total: 19m15s

[roborev-ci]

roborev: Combined Review (cb0275a)

Medium-severity issue found in subset export merge handling; no high or critical findings.

Medium

• internal/store/subset.go:982 — Alias cleanup can leave an incomplete merge packet

  Aliases for omitted merges are deleted only after merge-packet completeness is validated. A later merge using the same survivor may remain selected because its snapshot still contains an earlier merge’s alias; cleanup then removes the live alias while leaving it in the exported snapshot. This can produce an incomplete packet, expose provenance intended to be omitted, and break retired-UID resolution.

  Suggested fix: Perform alias pruning and packet-completeness checks to a fixed point before copying packets, or treat survivor-side aliases from earlier merges as explicit dependencies that cause the later packet to be omitted.

---

Reviewers: 2 done | Synthesis: codex, 14s | Total: 26m44s

[roborev-ci]

roborev: Combined Review (52920be)

Code review found one medium-severity correctness issue in split validation; no security issues were identified.

Medium

• internal/store/person_splits.go:380 — Post-merge aliases can be stranded during an “exact reversal.” Split validation considers only the merge-time person_merge_participants ledger. If a new participant is later linked to an absorbed-origin participant, it becomes bound to the survivor without entering that ledger. Selecting all recorded absorbed participants is then incorrectly accepted as an exact reversal: the split deletes the new identity link at line 434, moves only recorded participants, and leaves the new participant stranded on the source. The caller cannot include it because validation rejects non-ledger participants.
  • Suggested fix: Propagate new bindings into active merge lineages with the connected component’s unambiguous origin, or invalidate exact reversal and provide a supported way to select those additions. Add coverage for merging, linking a new alias to the absorbed side, and splitting while preserving that alias association.

---

Reviewers: 2 done | Synthesis: codex, 13s | Total: 25m12s

[roborev-ci]

roborev: Combined Review (1fc949a)

One medium-severity correctness issue must be addressed before merge.

Medium

• internal/store/person_merges.go:1940 — Consolidating participants from opposite sides of a reversible merge collapses their lineage to absorbed. If another survivor participant remains, validatePersonSplitLineage may incorrectly classify a later split as exact and move the mixed participant—including survivor-origin data—to the restored absorbed profile.
  • Fix: Preserve mixed-origin lineage or reject cross-origin participant consolidation. Exact split validation must treat mixed lineage as partial or ambiguous. Add a regression test covering this sequence.

---

Reviewers: 2 done | Synthesis: codex, 10s | Total: 20m9s

[roborev-ci]

roborev: Combined Review (b658604)

Code review found two medium-severity data-integrity issues in exact person splits.

Medium

• internal/store/person_splits.go:1062 — Recreated person_relationship_reviews rows do not apply the per-column dependency validation used for updates. A deduplicated review may be reinserted with an accepted_relationship_id referencing a relationship skipped because its type was deleted after the merge, causing the split to fail with a foreign-key error. Validate optional foreign keys during insert construction and set missing dependencies to NULL; add coverage for this scenario.

• internal/store/person_splits.go:526 — An exact split can restore a merge-collapsed generated identity candidate after its supporting source has been removed. Although support rows are skipped, the unsupported candidate and evidence remain. Require surviving source support or a durable user decision before restoration, or reconcile unsupported candidates before committing the split.

---

Reviewers: 2 done | Synthesis: codex, 9s | Total: 21m46s

[roborev-ci]

roborev: Combined Review (dc3a403)

Review verdict: Two medium-severity correctness issues remain in subset alias pruning and split evidence restoration.

Medium

• internal/store/subset.go:868 — Alias pruning depends on unrelated attribute inclusion

  Alias pruning runs only when profiles, attributes, and vCard resources are all included. A subset containing profiles and vCard resources but excluding attributes copies merge-created person_uid_aliases while omitting their merge packets, allowing retired UIDs to resolve without the associated lineage.

  Fix: Run alias pruning whenever vCard-resource aliases are copied, independently of whether complete merge packets are eligible. Add a subset behavior test covering this option combination.

• internal/store/person_splits.go:711 — Evidence support is checked at candidate level instead of per row

  Unsupported evidence is inferred from whether its parent candidate has any surviving source. When a generated candidate has multiple sources but an individual evidence row was supported only by a subsequently removed source, splitting restores stale evidence without a corresponding evidence-source row.

  Fix: Determine support independently for each generated evidence row and skip evidence whose current or restorable source support no longer exists.

---

Reviewers: 2 done | Synthesis: codex, 10s | Total: 13m29s

[roborev-ci]

roborev: Combined Review (ef492e1)

Code review identified one medium-severity lineage integrity issue; no security regressions were found.

Medium

• internal/store/person_merges.go:1942 — Participant consolidation validates differing origin_side values but not differing split_id states. Consolidating same-origin participants from separate splits retains only one split_id through COALESCE and deletes the other lineage row, corrupting split attribution and future lineage decisions. Treat (origin_side, split_id) as the complete lineage state and reject consolidation when either differs. Add coverage for participants from distinct partial splits.

---

Reviewers: 2 done | Synthesis: codex, 8s | Total: 21m33s

[roborev-ci]

roborev: Combined Review (40bbd35)

Code changes require one medium-severity fix before approval.

Medium

• internal/store/person_merge_inspection.go:451 — Accepting a merge candidate bypasses writableAttributeDefinition, allowing replacement values for inactive, derived, or otherwise API-immutable definitions. Validate the loaded definition before closing the current value, return an appropriate candidate conflict without mutation, and add coverage for acceptance after definition deactivation.

---

Reviewers: 2 done | Synthesis: codex, 6s | Total: 24m50s

[roborev-ci]

roborev: Combined Review (ae074b6)

Code review found one medium-severity provenance issue; no high or critical findings.

Medium

• internal/store/person_splits.go:785 — identity_match_evidence rows are checked only against unsupportedCandidates, not unsupportedEvidence. If a merge collapses a candidate’s endpoints and later removes the evidence’s sole source while the candidate retains independent support, an exact split can recreate stale evidence without provenance support.
  • Fix: Handle identityMatchEvidenceTableName separately and skip rows when either the candidate or the evidence ID is unsupported. Add coverage for endpoint-collapsed, independently supported candidates whose evidence is unsupported.

---

Reviewers: 2 done | Synthesis: codex, 13s | Total: 22m16s

[roborev-ci]

roborev: Combined Review (5b10699)

Verdict: One medium-severity correctness issue found; no material security regressions identified.

Medium

• internal/store/subset.go:1351 — Merge-packet validation incorrectly treats relationship_type_id and organization attribute definition_id as stable when it only verifies that the original numeric ID exists in the destination. Because these catalogs are remapped by universal_id during subset creation, gaps or reordered custom definitions/types can cause the old ID to reference a different row, producing an internally inconsistent immutable snapshot.
  • Fix: Resolve these dependencies through their source-to-destination universal_id mappings and omit the merge packet whenever the numeric ID was remapped, as already done for communication services and journaled person attributes.

---

Reviewers: 2 done | Synthesis: codex, 9s | Total: 20m4s

[roborev-ci]

roborev: Combined Review (7d46022)

Code review identified one medium-severity referential-integrity issue; no security boundary regressions were found.

Medium

• Pending merge references can become dangling — internal/store/persons.go:223, internal/store/person_splits.go:635

  The deletion guard checks owners of candidate attribute values but not person IDs referenced by pending record-reference values. After a merge makes the absorbed value inactive, its referenced person can be deleted. Accepting the candidate then fails, while an exact split can reactivate the value with a dangling value_record_id.

  Suggested fix: Treat referenced person targets in pending merge candidates as deletion dependencies, or invalidate affected candidates. Before split reactivation, validate record-reference targets and leave unsupported rows inactive. Add tests covering deletion of a candidate’s target before acceptance and before an exact split.

---

Reviewers: 2 done | Synthesis: codex, 9s | Total: 17m48s

[roborev-ci]

roborev: Combined Review (bf074b1)

Code review identified one medium-severity data-integrity issue in exact person splits.

Medium

• internal/store/person_splits.go:826 — Snapshot restoration does not validate nullable external dependencies. A deduplicated relationship review can retain a historical matched_person_id referencing a person deleted after the merge, causing an exact split to fail on a stale foreign key. Collapsed identity candidates have the same problem when their referenced service_id has been deleted.
  • Fix: When recreating rows, null missing external targets while continuing to remap survivor/absorbed person IDs to their split destinations. Add behavioral tests covering deletion of these dependencies between merge and split.

---

Reviewers: 2 done | Synthesis: codex, 8s | Total: 24m30s

[roborev-ci]

roborev: Combined Review (2f87405)

Code review found one medium-severity correctness issue in person-reference restoration; no security issues were identified.

Medium

• internal/store/person_splits.go:1142 — The three-way restoration logic treats person-reference columns as ordinary fields when they point outside the two split roots. If an externally referenced person is merged after the original merge, splitting can restore the deleted historical person ID, potentially rolling back the later merge or leaving dangling foreign-key-backed, polymorphic attribute, or candidate references.
  • Suggested fix: Exclude all registered person-reference columns from generic field restoration. Resolve historical external person IDs through their current merge lineage while remapping only the split roots.

---

Reviewers: 2 done | Synthesis: codex, 9s | Total: 32m11s

[roborev-ci]

roborev: Combined Review (c8d91db)

No issues found.

---

Reviewers: 2 done | Synthesis: codex | Total: 18m37s

[salmonumbrella]

@wesm ready for you

[wesm]

roger

[roborev-ci]

roborev: Combined Review (4baa6b0)

Merge/split functionality looks broadly sound, but one medium-severity concurrency gap can cause valid PostgreSQL merges to abort.

Medium

• internal/store/person_merges.go:325 — Merge snapshots are not isolated from all covered writers. SetPersonTrackingContext can delete person_tracking rows without the identity/person lock, and DeleteDailyNoteEntryContext can similarly cascade-delete daily_note_entry_persons. On PostgreSQL, either deletion may commit after snapshot capture but before reconciliation, leaving a journal entry whose row is missing and causing a valid merge to abort with ErrPersonMergeInvalid.
  • Fix: Make both mutations follow the merge lock ordering, preferably by acquiring the identity-mutation lock inside a transaction. Add PostgreSQL interleaving tests for deletions occurring after snapshot capture.

---

Reviewers: 2 done | Synthesis: codex, 7s | Total: 35m48s

[roborev-ci]

roborev: Combined Review (906aa6b)

One medium-severity issue remains in sequential partial split handling.

Medium

• internal/store/person_splits.go:238 — current_person_id is cleared only for exact reversals. When absorbed participants from a chained merge are split through multiple partial requests, the final split remains non-exact because earlier participants already have split_id. The merge therefore stays active indefinitely, causing DeletePerson to reject the source profile with ErrPersonMergeActive even though no unsplit absorbed lineage remains.
  • Suggested fix: After each split, clear current_person_id when no absorbed participants remain unsplit, or change the deletion guard to check for unsplit absorbed lineage. Add a behavioral test covering sequential partial splits followed by source-profile deletion.

---

Reviewers: 2 done | Synthesis: codex, 7s | Total: 14m12s

[roborev-ci]

roborev: Combined Review (0c5ccad)

Medium-severity issue found in chained partial-split lineage handling.

Medium

• internal/store/person_splits.go:233 — A partial split marks lineage rows only for the requested merge. In a chained merge such as A ← B, then C ← (A+B), splitting only B from the second merge leaves the first merge active under C, even though B is now bound elsewhere. The first merge can no longer be reversed and continues blocking deletion of C.
  • Fix: Mark selected participants as split across every active merge owned by the source profile, closing each merge whose absorbed lineage is fully split. Add coverage for this chained partial-split sequence.

---

Reviewers: 2 done | Synthesis: codex, 8s | Total: 13m20s

[roborev-ci]

roborev: Combined Review (ea8730a)

One medium-severity issue could cause valid subset exports to fail foreign-key validation.

Medium

• internal/store/subset.go:888 — Merge-packet selection is not closed over person_merge_participants.split_id. A retained packet can reference a split owned by another packet that completeness pruning removes. Because only splits whose own merge_id remains selected are copied, the final foreign-key check rejects an otherwise valid subset.
  • Fix: Include owning merge packets for referenced split IDs, or prune dependent packets to a fixed point. Add a subset test for a cross-lineage partial split where one packet is independently omitted.

---

Reviewers: 2 done | Synthesis: codex, 8s | Total: 20m17s

[roborev-ci]

roborev: Combined Review (3bd6983)

Changes requested: one high-severity data-restoration bug and two medium-severity API/export issues remain.

High

• Chained partial splits can permanently strand profile data — internal/store/person_splits.go:233
  A chained partial split marks participants as split across every active merge and closes earlier merge lineages, but restores rows only from the explicitly requested merge. For example, after A absorbs B and then C absorbs A/B, splitting B through the second merge closes the first merge without restoring B’s names, attributes, or other profile rows. Those rows remain assigned to the source, and the first merge can no longer be reversed.
  Fix: Replay each newly completed ancestor merge’s journal into the new person before closing propagated lineages, or keep each lineage active until its rows can be restored. Add a chained-split regression test containing absorbed profile data.

Medium

• Merge-required responses violate the OpenAPI oneOf contract — internal/api/person_merges.go:411
PersonMergeRequiredError also satisfies ErrorResponse, because the latter requires only error and permits additional properties. The response therefore matches both branches, potentially preventing generated clients from exposing profiles.
  Fix: Use anyOf, or make the alternatives mutually exclusive through a discriminator, constant error value, or stricter schemas.

• Subset exports silently omit merge packets for a documented flag combination — internal/store/subset.go:874
  Merge packets are copied only when IncludeAttributes is enabled, although the CLI permits --include-vcard-resources --include-profiles without it and says that combination copies complete merge packets. The packets are silently omitted, and the omitted count remains zero because counting is guarded by the same condition.
  Fix: Require IncludeAttributes when vCard resources or merge packets are requested, or report the dependency and omitted packet count accurately.

---

Reviewers: 2 done | Synthesis: codex, 14s | Total: 12m59s

[roborev-ci]

roborev: Combined Review (955ef4c)

The review found two medium-severity correctness issues in merge/split lineage restoration and identity-conflict rollback; no material security vulnerabilities were identified.

Medium

• internal/store/person_splits.go:640 — Incomplete profile restoration after chained merges

  Exact profile restoration is incorrectly coupled to survivorLineageIntact. If A absorbs B and then C, splitting B first makes the second merge inexact. Subsequently splitting all of C’s absorbed lineage moves only C’s participants, leaving C’s profile rows and retired-UID alias on A and permanently closing the merge.

  Fix: Track “all absorbed lineage selected” separately from exact reversal. Use it to restore absorbed-origin rows, finalize candidates, and retarget the absorbed UID alias, while continuing to report ExactReversal=false when the survivor lineage changed. Add coverage with profile data on C.

• internal/store/identity_match_apply.go:121 — Failed conflict acceptance does not fully restore internal state

  Restoring an identity-match decision after a binding conflict clears pre_conflict_state without restoring observation_conflict_origin. Accepting an observation-derived conflict across two profiles can therefore mutate the candidate despite returning a merge-required response; later observation cleanup can no longer demote it or recover its prior decision.

  Fix: Snapshot and restore both internal conflict columns alongside the public decision fields. Add a test confirming that a failed acceptance leaves an observation-derived conflict behaviorally unchanged.

---

Reviewers: 2 done | Synthesis: codex, 10s | Total: 22m37s

[roborev-ci]

roborev: Combined Review (48da12c)

Two medium-severity issues found involving rollback concurrency and subset export completeness.

Medium

• internal/store/identity_match_apply.go:69 — Rollback state is read before acquiring the identity lock. If another writer changes the candidate before acceptance, a later binding conflict can restore stale state and overwrite the intervening decision.

  • Fix: Capture the actual pre-transition candidate within the locked decision transaction and use it for rollback. Add a concurrency test where rejection acquires the lock before an acceptance that encounters a binding conflict.

• internal/store/subset.go:892 — Packet selection requires current_person_id to reference a copied person. Fully split merges set this field to NULL, causing complete merge packets and split history to be omitted even when all privacy options and dependencies are satisfied.

  • Fix: Select closed merges through copied split records or participant lineage as well, while retaining existing completeness checks. Test a fully reversed merge whose two resulting people are included in the subset.

---

Reviewers: 2 done | Synthesis: codex, 9s | Total: 17m45s

[roborev-ci]

roborev: Combined Review (ea5b3eb)

Code review passed with no Medium, High, or Critical findings.

---

Reviewers: 2 done | Synthesis: codex, 5s | Total: 13m16s

[roborev-ci]

roborev: Combined Review (e78e5bb)

No Medium, High, or Critical findings; reviewers found no material security regressions.

---

Reviewers: 2 done | Synthesis: codex, 6s | Total: 24m15s