Skip to content

fix(contacts): add view action, show phones in list, preserve fields on update#4748

Open
holden093 wants to merge 3 commits into
odysseus-dev:devfrom
holden093:fix/manage-contact-list-phones
Open

fix(contacts): add view action, show phones in list, preserve fields on update#4748
holden093 wants to merge 3 commits into
odysseus-dev:devfrom
holden093:fix/manage-contact-list-phones

Conversation

@holden093

@holden093 holden093 commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes three gaps in the manage_contacts agent tool that made phone-number workflows unreliable: list now includes phone numbers, a new view action returns full contact details by UID, and update preserves existing emails/phones when the caller doesn't pass them (preventing data loss on partial edits like renaming).

Target branch

  • This PR targets dev, not main.

Linked Issue

Fixes #4747

Type of Change

  • Bug fix (non-breaking — fixes a confirmed issue)

Checklist

  • I searched open issues and open PRs — this is not a duplicate.
  • This PR targets dev
  • My changes are limited to the scope described above — no unrelated refactors or whitespace changes mixed in.
  • I actually ran the app (docker compose up or uvicorn app:app) and verified the change works end-to-end. Type-checks and unit tests are not enough.

How to Test

  1. Add a contact with name, email, and phone number via the UI or CardDAV.
  2. Ask the agent to "list my contacts" — verify phone numbers appear in the output.
  3. Ask the agent to "view contact [name]" — verify full details (phones, address, UID) are shown.
  4. Ask the agent to "rename [contact] to [new name]" — verify the phone and email are preserved after the update.

Visual / UI changes

None — backend-only change.

@github-actions github-actions Bot added the ready for review Description complete — ready for maintainer review label Jun 22, 2026

@vdmkenny vdmkenny left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good direction (phones in list, the view action, and preserving fields on rename are all real gaps), but the preserve logic still loses data on partial single-field updates, which is the common case it is meant to cover.

The existing-contact fetch only runs when emails, email, and phones are all absent:

existing = None
if "emails" not in args and "email" not in args and "phones" not in args:
    rows = await asyncio.to_thread(cc._fetch_contacts)
    existing = next((r for r in rows if r.get("uid") == uid), None)
...
if "phones" in args:
    phones = [...]
else:
    phones = list(existing.get("phones") or []) if existing else []

So update {uid, emails:["new@x"]} with no phones takes the else branch while existing is still None, and phones get wiped. Same in reverse: passing only phones wipes the emails. The data loss it claims to prevent still happens whenever the caller updates one of the two fields.

Fix: fetch existing whenever either field is missing (or just always on update), then fall back per-field independently:

existing = None
if "emails" not in args and "email" not in args or "phones" not in args:
    rows = await asyncio.to_thread(cc._fetch_contacts, True)
    existing = next((r for r in rows if r.get("uid") == uid), None)

Two smaller things:

  • The preserve fetch uses _fetch_contacts() (cache, force=False), so a contact added moments earlier may be missing from the cache, giving existing=None and the same wipe. view already passes force=True; do the same here.
  • Please add a test: rename-only keeps both emails and phones; updating only emails keeps the existing phones (and vice versa). That pins exactly this bug.

list/view/phones changes are fine as-is.

@alteixeira20 alteixeira20 added the bug Something isn't working label Jun 26, 2026
@holden093
holden093 force-pushed the fix/manage-contact-list-phones branch from f34a75e to aed606e Compare June 26, 2026 16:25
@holden093

Copy link
Copy Markdown
Contributor Author

Addressed all three issues, @vdmkenny:

  1. Preserve guard fixed — changed the condition from AND to OR: fetch the existing contact whenever either emails or phones is missing, not only when both are absent. Single-field updates like {uid, emails:[new]} now correctly preserve the other field.

  2. Force-refresh on preserve fetch — switched _fetch_contacts() to _fetch_contacts(True) so a just-added contact is not missed by a stale cache.

  3. Tests added — four tests in test_contacts_update_preserve.py covering: rename-only keeps both emails and phones, emails-only update preserves phones, phones-only update preserves emails, and explicit both-fields overwrites without a fetch. All pass.

@holden093
holden093 force-pushed the fix/manage-contact-list-phones branch 2 times, most recently from 74ed6f0 to 3ac0876 Compare July 5, 2026 18:41
holden093 added 2 commits July 7, 2026 21:33
…on update

- Add 'view' action to manage_contact: fetch a single contact by UID and
  display name, UID, emails, phones, and address.
- Show phone numbers in 'list' output alongside emails.
- Preserve existing emails/phones during partial update (rename-only,
  single-field updates) by fetching the current contact when either
  field is missing. Uses force=True on _fetch_contacts to avoid a stale
  cache masking a just-added contact.
- Fix the preserve guard: use OR instead of AND so single-field updates
  like {uid, emails:[new]} don't wipe phones and vice versa.
@holden093
holden093 force-pushed the fix/manage-contact-list-phones branch from 3ac0876 to e893d14 Compare July 7, 2026 19:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working ready for review Description complete — ready for maintainer review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(contacts): manage_contacts tool hides phone numbers and has no view action

3 participants