feat(contacts): apply duplicate merge plans#817
Conversation
|
Codex review: needs changes before merge. Reviewed June 15, 2026, 8:56 AM ET / 12:56 UTC. Summary Reproducibility: yes. for the review finding: source inspection shows same normalized email/phone values are de-duplicated by value only, and the new test already expects a same-value email collapse. For the feature request itself, reproduction is not a bug path because current main is documented as preview-only. Review metrics: 3 noteworthy metrics.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Copy recommended automerge instructionNext step before merge
Security Review findings
Review detailsBest possible solution: Keep the guarded apply design, but refuse or deliberately merge same-value email/phone conflicts before deletion and leave release-owned changelog updates to the release flow. Do we have a high-confidence way to reproduce the issue? Yes for the review finding: source inspection shows same normalized email/phone values are de-duplicated by value only, and the new test already expects a same-value email collapse. For the feature request itself, reproduction is not a bug path because current main is documented as preview-only. Is this the best way to solve the issue? No. The implementation is directionally aligned, but the narrow safe fix is to refuse or preserve same-value email/phone fields with differing mutable attributes before deleting the redundant contact. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against f212bc5f8970. Label changesLabel changes:
Label justifications:
Evidence reviewedAcceptance criteria:
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 90aed08591
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if key == "" || seen[key] { | ||
| continue |
There was a problem hiding this comment.
Preserve differing labels on duplicate email/phone values
When keyFn is used for emailAddresses or phoneNumbers, this branch drops every later item with the same normalized value. If a redundant contact has the same email/number but a different mutable label/type or other user-entered attributes, --apply will keep only the primary's item and then delete the redundant contact, silently losing that metadata; conflicting same-key items should be merged deliberately or cause the group to be refused.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 330dbd55be
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if decodeErr := json.Unmarshal(data, &object); decodeErr != nil { | ||
| return nil, "", fmt.Errorf("decode contact field: %w", decodeErr) | ||
| } | ||
| delete(object, "metadata") |
There was a problem hiding this comment.
Preserve primary field metadata during merges
When an applied group contains a multi-valued field with a user-selected source primary (for example a non-first primary email/phone), this deletes the entire field metadata object before updateContact replaces that field, so the PATCH no longer carries the People API metadata.sourcePrimary marker documented for source-level primary fields (https://developers.google.com/people/api/rest/v1/people#FieldMetadata). The merge can therefore silently change which email/phone/URL is primary even though the values were copied; strip only the read-only/source bits while retaining one appropriate sourcePrimary marker.
Useful? React with 👍 / 👎.
Closes #815
Summary
contacts dedupe --apply/--merge--resource people/...scoping for exact reviewed automation targetsValidation
autoreview --mode local --parallel-tests 'make ci': clean, no actionable findingsmake ci: passclawdbot@gmail.com: created two disposable duplicate contacts, verified exact-scoped dry-run, applied one merge/delete, read back both phone values from the primary, then cleaned up