diff --git a/src/tool_schemas.py b/src/tool_schemas.py index 7585f3e9d..b8e256ed6 100644 --- a/src/tool_schemas.py +++ b/src/tool_schemas.py @@ -1079,12 +1079,12 @@ FUNCTION_TOOL_SCHEMAS = [ "type": "function", "function": { "name": "manage_contact", - "description": "Create, update, delete, or list the user's CardDAV contacts. Use to save a new contact, update an existing one (email/phone/address), or remove one. Add does not require email: name + phone or name + address is valid. For update/delete you need the contact's uid — call action='list' first to find it. Writes go through the same dedupe + validation as the Contacts UI.", + "description": "Create, update, delete, view, or list the user's CardDAV contacts. Use to save a new contact, update an existing one (email/phone/address), view full details by uid, or remove one. For update/delete/view you need the contact's uid — call action='list' first to find it. Writes go through the same dedupe + validation as the Contacts UI. Update preserves existing emails/phones when they aren't passed — safe for partial edits like renaming.", "parameters": { "type": "object", "properties": { - "action": {"type": "string", "enum": ["list", "add", "update", "delete"], - "description": "list = show all contacts (with uids); add = create; update = edit by uid; delete = remove by uid."}, + "action": {"type": "string", "enum": ["list", "view", "add", "update", "delete"], + "description": "list = show all contacts (with uids and phones); view = show full details of one contact by uid; add = create; update = edit by uid (preserves fields not passed); delete = remove by uid."}, "uid": {"type": "string", "description": "Contact UID (required for update/delete; get it from action=list)."}, "name": {"type": "string", "description": "Contact's display name (for add/update)."}, "email": {"type": "string", "description": "Single email address (convenience for add, or the primary email for update). Optional when phone or address is provided."}, diff --git a/src/tools/contacts.py b/src/tools/contacts.py index fa9e84d6d..fec55c6af 100644 --- a/src/tools/contacts.py +++ b/src/tools/contacts.py @@ -103,9 +103,28 @@ async def do_manage_contact(content: str, owner: Optional[str] = None) -> Dict: lines = [f"{len(rows)} contacts:"] for c in rows: em = ", ".join(c.get("emails") or []) - lines.append(f"- {c.get('name') or '(no name)'} <{em}> [uid={c.get('uid','')}]") + ph = ", ".join(c.get("phones") or []) + detail = f"<{em}>" if em else "" + if ph: + detail += f" phone: {ph}" + lines.append(f"- {c.get('name') or '(no name)'} {detail} [uid={c.get('uid','')}]") return {"output": "\n".join(lines), "exit_code": 0} + if action == "view": + uid = (args.get("uid") or "").strip() + if not uid: + return {"error": "uid is required for view (use action=list to find it)", "exit_code": 1} + rows = await asyncio.to_thread(cc._fetch_contacts, True) + c = next((r for r in rows if r.get("uid") == uid), None) + if not c: + return {"output": f"No contact found with uid={uid}.", "exit_code": 0} + detail = f"Name: {c.get('name') or '(no name)'}\n" + detail += f"UID: {c.get('uid','')}\n" + detail += f"Emails: {', '.join(c.get('emails') or ['(none)'])}\n" + detail += f"Phones: {', '.join(c.get('phones') or ['(none)'])}\n" + detail += f"Address: {c.get('address') or '(none)'}" + return {"output": detail, "exit_code": 0} + if action == "add": email = (args.get("email") or "").strip() phones = [str(p or "").strip() for p in (args.get("phones") or []) if str(p or "").strip()] @@ -136,14 +155,28 @@ async def do_manage_contact(content: str, owner: Optional[str] = None) -> Dict: if not uid: return {"error": "uid is required for update (use action=list to find it)", "exit_code": 1} name = (args.get("name") or "").strip() - emails = args.get("emails") - if emails is None and args.get("email"): - emails = [args["email"]] - emails = [e.strip() for e in (emails or []) if e and e.strip()] - phones = [p.strip() for p in (args.get("phones") or []) if p and p.strip()] - address = (args.get("address") or "").strip() - if not name and not emails and not phones and not address: - return {"error": "Provide a name, emails, phones, or address to update", "exit_code": 1} + # Preserve existing emails/phones when caller passes neither — + # prevents data loss on partial updates like "rename Lisa". + # Fetch whenever *either* field is missing (not just when both + # are absent), so single-field updates like {uid, emails:[new]} + # don't wipe the other field. + 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) + if "emails" in args or "email" in args: + emails = args.get("emails") + if emails is None and args.get("email"): + emails = [args["email"]] + emails = [e.strip() for e in (emails or []) if e and e.strip()] + else: + emails = list(existing.get("emails") or []) if existing else [] + if "phones" in args: + phones = [p.strip() for p in (args.get("phones") or []) if p and p.strip()] + else: + phones = list(existing.get("phones") or []) if existing else [] + if not name and not emails: + return {"error": "Provide a name or emails to update", "exit_code": 1} if not name and emails: name = emails[0].split("@")[0] ok = await asyncio.to_thread(cc._update_contact, uid, name, emails, phones, address) @@ -156,6 +189,6 @@ async def do_manage_contact(content: str, owner: Optional[str] = None) -> Dict: ok = await asyncio.to_thread(cc._delete_contact, uid) return {"output": "Contact deleted." if ok else "Delete failed.", "exit_code": 0 if ok else 1} - return {"error": f"Unknown action '{action}'. Use list, add, update, or delete.", "exit_code": 1} + return {"error": f"Unknown action '{action}'. Use list, view, add, update, or delete.", "exit_code": 1} except Exception as e: return {"error": f"Contact operation failed: {e}", "exit_code": 1} diff --git a/tests/test_contacts_update_preserve.py b/tests/test_contacts_update_preserve.py new file mode 100644 index 000000000..96440c8fa --- /dev/null +++ b/tests/test_contacts_update_preserve.py @@ -0,0 +1,131 @@ +"""Pin the contact-update preserve logic: partial updates must not wipe +the other field. + +The regression: {uid, emails:[new]} with no phones would skip the +existing-contact fetch (AND guard) and silently drop phones. Same in +reverse. Fixed by fetching whenever *either* field is missing (OR guard) +and using `force=True` so a just-added contact isn't missed by a stale +cache. +""" +import asyncio + +import pytest + + +def _stub_cc(monkeypatch, contacts_by_uid, *, calls=None): + """Install a fake contacts_routes module that serves `contacts_by_uid`. + + Returns a ``calls`` list of (action, *args) tuples for assertions. + """ + import sys + import types + + call_log = calls if calls is not None else [] + + class FakeCC(types.ModuleType): + pass + + mod = FakeCC("routes.contacts_routes") + mod._fetch_contacts = lambda force=False: list(contacts_by_uid.values()) + mod._update_contact = lambda uid, name, emails, phones, **kw: call_log.append( + ("update", uid, name, emails, phones) + ) or True + mod._create_contact = lambda name, email, **kw: call_log.append( + ("create", name, email) + ) or True + mod._delete_contact = lambda uid: call_log.append(("delete", uid)) or True + + monkeypatch.setitem(sys.modules, "routes.contacts_routes", mod) + # Also patch _fetch_contacts on the real module (just in case) + import routes.contacts_routes as real_cc + monkeypatch.setattr(real_cc, "_fetch_contacts", mod._fetch_contacts) + monkeypatch.setattr(real_cc, "_update_contact", mod._update_contact) + + return call_log + + +@pytest.fixture +def contact_bob(): + return { + "uid": "bob-001", + "name": "Bob", + "emails": ["bob@example.com"], + "phones": ["+1-555-0100"], + "address": "123 Main", + } + + +# ------------------------------------------------------------------ +# Rename-only: both emails + phones survive +# ------------------------------------------------------------------ +def test_update_rename_preserves_emails_and_phones(monkeypatch, contact_bob): + call_log = _stub_cc(monkeypatch, {"bob-001": contact_bob}) + + from src.tools.contacts import do_manage_contact + result = asyncio.run( + do_manage_contact('{"action":"update","uid":"bob-001","name":"Robert"}') + ) + + assert result.get("output") == "Contact updated." + assert len(call_log) == 1 + _, uid, name, emails, phones = call_log[0] + assert name == "Robert" + assert emails == ["bob@example.com"] # preserved + assert phones == ["+1-555-0100"] # preserved + + +# ------------------------------------------------------------------ +# Update only emails → phones survive +# ------------------------------------------------------------------ +def test_update_emails_only_preserves_phones(monkeypatch, contact_bob): + call_log = _stub_cc(monkeypatch, {"bob-001": contact_bob}) + + from src.tools.contacts import do_manage_contact + result = asyncio.run( + do_manage_contact( + '{"action":"update","uid":"bob-001","emails":["new@example.com"]}' + ) + ) + + assert result.get("output") == "Contact updated." + _, uid, name, emails, phones = call_log[0] + assert emails == ["new@example.com"] + assert phones == ["+1-555-0100"] # preserved, not wiped + + +# ------------------------------------------------------------------ +# Update only phones → emails survive +# ------------------------------------------------------------------ +def test_update_phones_only_preserves_emails(monkeypatch, contact_bob): + call_log = _stub_cc(monkeypatch, {"bob-001": contact_bob}) + + from src.tools.contacts import do_manage_contact + result = asyncio.run( + do_manage_contact( + '{"action":"update","uid":"bob-001","phones":["+1-555-9999"]}' + ) + ) + + assert result.get("output") == "Contact updated." + _, uid, name, emails, phones = call_log[0] + assert phones == ["+1-555-9999"] + assert emails == ["bob@example.com"] # preserved, not wiped + + +# ------------------------------------------------------------------ +# Update both fields explicitly → no fetch needed, both overwritten +# ------------------------------------------------------------------ +def test_update_both_fields_explicitly_does_not_fetch(monkeypatch, contact_bob): + call_log = _stub_cc(monkeypatch, {"bob-001": contact_bob}) + + from src.tools.contacts import do_manage_contact + result = asyncio.run( + do_manage_contact( + '{"action":"update","uid":"bob-001","emails":["a@x.com"],"phones":["+0"]}' + ) + ) + + assert result.get("output") == "Contact updated." + _, uid, name, emails, phones = call_log[0] + assert emails == ["a@x.com"] + assert phones == ["+0"]