Merge be4215c699 into 20e7fc0164

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"],
+                    "action": {"type": "string", "enum": ["list", "view", "add", "update", "delete"],
-                               "description": "list = show all contacts (with uids); add = create; update = edit by uid; delete = remove by uid."},
+                               "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."},

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")
+            # Preserve existing emails/phones when caller passes neither —
-            if emails is None and args.get("email"):
+            # prevents data loss on partial updates like "rename Lisa".
-                emails = [args["email"]]
+            # Fetch whenever *either* field is missing (not just when both
-            emails = [e.strip() for e in (emails or []) if e and e.strip()]
+            # are absent), so single-field updates like {uid, emails:[new]}
-            phones = [p.strip() for p in (args.get("phones") or []) if p and p.strip()]
+            # don't wipe the other field.
-            address = (args.get("address") or "").strip()
+            existing = None
-            if not name and not emails and not phones and not address:
+            if "emails" not in args and "email" not in args or "phones" not in args:
-                return {"error": "Provide a name, emails, phones, or address to update", "exit_code": 1}
+                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}

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"]