test(contacts): address PR #719 review feedback

- Type-annotate _wrap_contact_field signature; drop stale "url" mention
  from its docstring (url is handled by the list-coercion helper, not
  this one).
- Split the shape-coercion helper so comma-splitting only applies to
  categories: _as_str_list (no split) for org/nickname/url,
  _split_categories (comma split) for CATEGORIES. Fixes the case where
  organization="Smith, Jones & Associates" was mangled into a two-
  component ORG.
- Share _normalize_contact_data between create and update so
  _merge_vcard_properties only sees canonical keys; add URL handlers in
  both update branches so the primary update path no longer drops URL
  silently.
- Annotate the Contact(**kwargs) type:ignore with the reason
  (pythonvCard4 typeshed doesn't accept **dict[str, Any]).
- Add tests/unit/client/test_contacts.py (pure unit, no HTTP) covering
  the #716 round-trip, comma-in-org regression, invalid-bday warning,
  tel/phone precedence, categories string-vs-list behaviour, and direct
  _normalize_contact_data cases.
- Extend the MCP workflow test with an update-with-url step asserting
  the URL handler in _merge_vcard_properties.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

nextcloud_mcp_server/client/contacts.py

@@ -11,7 +11,8 @@ from .base import BaseNextcloudClient
 logger = logging.getLogger(__name__)
 
 
-# Keys that _build_contact_from_data consumes. Used to warn (not error) on unknown keys.
+# Canonical keys that _build_contact_from_data consumes. Aliases (``phone``, ``organization``)
+# are normalised to their canonical form by _normalize_contact_data before lookup.
 _SUPPORTED_CONTACT_KEYS = frozenset(
     {
         "fn",
@@ -30,8 +31,27 @@ _SUPPORTED_CONTACT_KEYS = frozenset(
 )
 
 
-def _wrap_contact_field(value) -> list[dict]:
-    """Normalize an email/tel/url input into pythonvCard4's list-of-dicts shape.
+def _normalize_contact_data(contact_data: dict) -> dict:
+    """Map documented aliases to canonical keys.
+
+    ``phone`` → ``tel``, ``organization`` → ``org``. The canonical key wins if both
+    are supplied, so callers who set ``tel`` don't lose it to a stray ``phone`` entry.
+    Returns a new dict — does not mutate the caller's argument.
+    """
+    normalised = dict(contact_data)
+    if "phone" in normalised and "tel" not in normalised:
+        normalised["tel"] = normalised.pop("phone")
+    else:
+        normalised.pop("phone", None)
+    if "organization" in normalised and "org" not in normalised:
+        normalised["org"] = normalised.pop("organization")
+    else:
+        normalised.pop("organization", None)
+    return normalised
+
+
+def _wrap_contact_field(value: str | dict | list | None) -> list[dict]:
+    """Normalize an email/tel input into pythonvCard4's list-of-dicts shape.
 
     Accepts a plain string, a dict already in ``{value, type}`` form, or a list of
     either. Empty strings are dropped. Always returns a list (possibly empty).
@@ -49,6 +69,29 @@ def _wrap_contact_field(value) -> list[dict]:
     return out
 
 
+def _as_str_list(value: str | list) -> list[str]:
+    """Wrap a bare string in a list. Does NOT split on commas.
+
+    Used for ORG/NICKNAME/URL where commas are part of the value (e.g.
+    ``"Smith, Jones & Associates"``) and only the list wrapper is needed to
+    prevent pythonvCard4 from iterating the string character-by-character.
+    """
+    return value if isinstance(value, list) else [value]
+
+
+def _split_categories(value: str | list) -> list[str]:
+    """Normalise CATEGORIES input: a comma-separated string is split into a list.
+
+    Unlike ORG/NICKNAME, CATEGORIES is canonically comma-separated in vCards
+    (``CATEGORIES:a,b,c``) so splitting a bare string is the expected shape.
+    Lists pass through unchanged — callers that already provide ``["a,b"]`` keep
+    their exact item, no double-splitting.
+    """
+    if isinstance(value, list):
+        return value
+    return [v.strip() for v in value.split(",") if v.strip()]
+
+
 def _build_contact_from_data(contact_data: dict, uid: str) -> Contact:
     """Build a pythonvCard4 Contact from an MCP ``contact_data`` dict.
 
@@ -56,47 +99,37 @@ def _build_contact_from_data(contact_data: dict, uid: str) -> Contact:
     library, normalising shapes (list/str) to avoid pythonvCard4's char-by-char
     iteration of bare strings — see issue #716.
     """
+    data = _normalize_contact_data(contact_data)
 
-    # pythonvCard4 iterates bare strings character-by-character for list-typed fields
-    # (ORG, NICKNAME, CATEGORIES, URL), producing garbage like ``ORG:A;c;m;e``. Wrap
-    # single strings in a list to keep the vCard well-formed.
-    def _as_list(value):
-        if isinstance(value, list):
-            return value
-        if isinstance(value, str) and "," in value:
-            return [v.strip() for v in value.split(",") if v.strip()]
-        return [value]
+    kwargs: dict = {"fn": data.get("fn"), "uid": uid}
 
-    kwargs: dict = {"fn": contact_data.get("fn"), "uid": uid}
-
-    emails = _wrap_contact_field(contact_data.get("email"))
+    emails = _wrap_contact_field(data.get("email"))
     if emails:
         kwargs["email"] = emails
 
-    tels = _wrap_contact_field(contact_data.get("tel") or contact_data.get("phone"))
+    tels = _wrap_contact_field(data.get("tel"))
     if tels:
         kwargs["tel"] = tels
 
-    org_value = contact_data.get("org") or contact_data.get("organization")
-    if org_value:
-        kwargs["org"] = _as_list(org_value)
+    if data.get("org"):
+        kwargs["org"] = _as_str_list(data["org"])
 
-    if contact_data.get("note"):
-        kwargs["note"] = contact_data["note"]
+    if data.get("note"):
+        kwargs["note"] = data["note"]
 
-    if contact_data.get("title"):
-        kwargs["title"] = contact_data["title"]
+    if data.get("title"):
+        kwargs["title"] = data["title"]
 
-    if contact_data.get("nickname"):
-        kwargs["nickname"] = _as_list(contact_data["nickname"])
+    if data.get("nickname"):
+        kwargs["nickname"] = _as_str_list(data["nickname"])
 
-    if contact_data.get("categories"):
-        kwargs["categories"] = _as_list(contact_data["categories"])
+    if data.get("categories"):
+        kwargs["categories"] = _split_categories(data["categories"])
 
-    if contact_data.get("url"):
-        kwargs["url"] = _as_list(contact_data["url"])
+    if data.get("url"):
+        kwargs["url"] = _as_str_list(data["url"])
 
-    bday = contact_data.get("bday")
+    bday = data.get("bday")
     if bday:
         if isinstance(bday, date):
             kwargs["bday"] = bday
@@ -106,10 +139,12 @@ def _build_contact_from_data(contact_data: dict, uid: str) -> Contact:
             except ValueError:
                 logger.warning("Ignoring non-ISO bday value: %r", bday)
 
-    unknown = set(contact_data) - _SUPPORTED_CONTACT_KEYS
+    unknown = set(data) - _SUPPORTED_CONTACT_KEYS
     if unknown:
         logger.debug("Ignoring unknown contact_data keys: %s", sorted(unknown))
 
+    # kwargs built dynamically from contact_data; pythonvCard4's Contact typeshed
+    # has specific typed params and doesn't accept **dict[str, Any].
     return Contact(**kwargs)  # type: ignore[arg-type]
 
 
@@ -251,6 +286,9 @@ class ContactsClient(BaseNextcloudClient):
         carddav_path = self._get_carddav_base_path()
         url = f"{carddav_path}/{addressbook}/{uid}.vcf"
 
+        # Canonicalise aliases up front so both code paths (merge + fallback) agree.
+        contact_data = _normalize_contact_data(contact_data)
+
         # Get raw vCard content to preserve all properties including extended ones
         raw_vcard_content = ""
         if not etag:
@@ -480,6 +518,17 @@ class ContactsClient(BaseNextcloudClient):
                 elif property_name == "TITLE" and "title" in contact_data:
                     updated_lines.append(f"TITLE:{contact_data['title']}")
                     updated_properties.add("title")
+                elif property_name == "URL" and "url" in contact_data:
+                    if "url" not in updated_properties:
+                        url_value = contact_data["url"]
+                        if isinstance(url_value, list):
+                            url_value = url_value[0] if url_value else ""
+                        if url_value:
+                            updated_lines.append(f"URL:{url_value}")
+                        updated_properties.add("url")
+                    else:
+                        # Keep additional URLs unchanged
+                        updated_lines.append(line)
                 else:
                     # Keep all other properties unchanged (preserves all extended/custom fields)
                     updated_lines.append(line)
@@ -511,6 +560,12 @@ class ContactsClient(BaseNextcloudClient):
                         updated_lines.append(f"ORG:{value}")
                     elif key == "title":
                         updated_lines.append(f"TITLE:{value}")
+                    elif key == "url":
+                        url_value = (
+                            value[0] if isinstance(value, list) and value else value
+                        )
+                        if url_value:
+                            updated_lines.append(f"URL:{url_value}")
 
             # Add the END:VCARD line
             updated_lines.append("END:VCARD")

tests/server/test_contacts_mcp.py

@@ -62,6 +62,24 @@ async def test_mcp_contacts_workflow(
         assert "ORG:MCP Test Corp" in raw_vcard
         assert f"NOTE:Created by test {unique_suffix}" in raw_vcard
 
+        # 4b. Update with a URL — regression guard for PR #719 review:
+        # _merge_vcard_properties previously had no URL handler, silently dropping it.
+        update_result = await nc_mcp_client.call_tool(
+            "nc_contacts_update_contact",
+            {
+                "addressbook": addressbook_name,
+                "uid": contact_uid,
+                "contact_data": {"url": "https://mcp-test.example.com"},
+            },
+        )
+        assert update_result.isError is False
+        contacts = await nc_client.contacts.list_contacts(addressbook=addressbook_name)
+        updated = next(c for c in contacts if c["vcard_id"] == contact_uid)
+        updated_vcard = updated.get("addressdata", "")
+        assert "mcp-test.example.com" in updated_vcard
+        # Prior properties must not have been clobbered by the merge.
+        assert "ORG:MCP Test Corp" in updated_vcard
+
         # 5. Delete contact via MCP
         logger.info(f"Deleting contact {contact_uid} via MCP")
         delete_c_result = await nc_mcp_client.call_tool(

tests/unit/client/test_contacts.py

@@ -0,0 +1,179 @@
+"""Unit tests for the contacts client vCard builder.
+
+These exercise ``_build_contact_from_data`` in isolation — no HTTP, no fixtures —
+so they cover the issue #716 regression surface and the edge cases flagged in
+PR #719 review without standing up the compose stack.
+"""
+
+from datetime import date
+
+import pytest
+
+from nextcloud_mcp_server.client.contacts import (
+    _build_contact_from_data,
+    _normalize_contact_data,
+)
+
+pytestmark = pytest.mark.unit
+
+
+def _vcard(**kwargs) -> str:
+    """Build a vCard from ``contact_data`` with a fixed uid, return the serialised text."""
+    return _build_contact_from_data(kwargs, uid="unit-test-uid").to_vcard()
+
+
+def test_issue_716_minimal_payload_keeps_all_fields():
+    """Reporter's exact payload from issue #716: every field must survive."""
+    vcard = _vcard(
+        fn="Repro User",
+        email="repro@example.com",
+        phone="555-0716",
+        organization="Acme Corp",
+        note="Issue 716",
+    )
+    assert "FN:Repro User" in vcard
+    assert "EMAIL" in vcard and "repro@example.com" in vcard
+    assert "TEL" in vcard and "555-0716" in vcard
+    assert "ORG:Acme Corp" in vcard
+    assert "NOTE:Issue 716" in vcard
+
+
+def test_org_preserves_comma_in_company_name():
+    """Regression: ``_as_list`` used to comma-split ORG, mangling names like
+    "Smith, Jones & Associates" into a two-component ORG. After the fix the whole
+    string is a single ORG component (with the comma RFC-6350-escaped as ``\\,``).
+    """
+    vcard = _vcard(fn="Alice", organization="Smith, Jones & Associates")
+    org_line = next(line for line in vcard.splitlines() if line.startswith("ORG"))
+    # Single component: no unescaped semicolon separator.
+    payload = org_line.split(":", 1)[1]
+    assert ";" not in payload
+    # Comma is escaped per RFC 6350 but the logical value is preserved.
+    assert payload.replace(r"\,", ",") == "Smith, Jones & Associates"
+
+
+def test_org_list_input_produces_structured_org():
+    """A list input is the opt-in shape for multi-component ORG (Company;Department)."""
+    vcard = _vcard(fn="Alice", org=["Acme", "Engineering"])
+    assert "ORG:Acme;Engineering" in vcard
+
+
+def test_invalid_bday_is_dropped_not_raised(caplog):
+    """An unparseable BDAY must warn and be omitted, not crash the call."""
+    import logging
+
+    with caplog.at_level(
+        logging.WARNING, logger="nextcloud_mcp_server.client.contacts"
+    ):
+        vcard = _vcard(fn="Alice", bday="not-a-date")
+    assert "BDAY" not in vcard
+    assert any("bday" in r.message.lower() for r in caplog.records)
+
+
+def test_valid_iso_bday_is_persisted():
+    vcard = _vcard(fn="Alice", bday="1990-05-01")
+    assert "BDAY:1990-05-01" in vcard
+
+
+def test_date_object_bday_is_persisted():
+    vcard = _vcard(fn="Alice", bday=date(1985, 12, 24))
+    assert "BDAY:1985-12-24" in vcard
+
+
+def test_tel_takes_precedence_over_phone_alias():
+    """When the caller supplies both canonical and alias, canonical wins. Documents
+    the precedence so future callers aren't surprised.
+    """
+    vcard = _vcard(fn="Alice", tel="111-1111", phone="222-2222")
+    assert "111-1111" in vcard
+    assert "222-2222" not in vcard
+
+
+def test_organization_alias_fills_in_when_org_absent():
+    vcard = _vcard(fn="Alice", organization="Acme")
+    assert "ORG:Acme" in vcard
+
+
+def test_categories_string_is_split_on_commas():
+    vcard = _vcard(fn="Alice", categories="friends,work,vip")
+    cat_line = next(
+        line for line in vcard.splitlines() if line.startswith("CATEGORIES")
+    )
+    assert cat_line == "CATEGORIES:friends,work,vip"
+
+
+def test_categories_list_passes_through_unchanged():
+    """A caller that already supplied a list shouldn't have their entries split again
+    — ``["friends,work"]`` stays as one item (with the comma RFC-6350-escaped), not
+    two categories ``friends`` + ``work``.
+    """
+    vcard = _vcard(fn="Alice", categories=["friends,work"])
+    cat_line = next(
+        line for line in vcard.splitlines() if line.startswith("CATEGORIES")
+    )
+    payload = cat_line.split(":", 1)[1]
+    assert payload == r"friends\,work"  # one item, comma escaped
+
+
+def test_nickname_bare_string_is_not_char_iterated():
+    """Regression: pythonvCard4 iterates bare strings; we wrap to prevent that."""
+    vcard = _vcard(fn="Alice", nickname="Bob")
+    nick_line = next(line for line in vcard.splitlines() if line.startswith("NICKNAME"))
+    assert nick_line == "NICKNAME:Bob"
+
+
+def test_url_bare_string_is_not_char_iterated():
+    vcard = _vcard(fn="Alice", url="https://example.com")
+    # Must appear as a single URL, not one URL: per character.
+    url_lines = [line for line in vcard.splitlines() if line.startswith("URL")]
+    assert url_lines == ["URL:https://example.com"]
+
+
+def test_unknown_keys_are_ignored_without_error(caplog):
+    """Future-compat: callers sending unknown keys shouldn't blow up."""
+    import logging
+
+    with caplog.at_level(logging.DEBUG, logger="nextcloud_mcp_server.client.contacts"):
+        vcard = _vcard(fn="Alice", totally_made_up_field="ignored")
+    assert "FN:Alice" in vcard
+    assert "totally_made_up_field" not in vcard
+    # A debug log is expected but not required — main guarantee is that no exception is raised.
+
+
+def test_empty_email_is_skipped():
+    """An empty string for email must not emit an EMAIL: line."""
+    vcard = _vcard(fn="Alice", email="")
+    assert "EMAIL" not in vcard
+
+
+def test_dict_form_email_preserves_custom_type():
+    vcard = _vcard(
+        fn="Alice",
+        email={"value": "work@example.com", "type": ["WORK"]},
+    )
+    assert "EMAIL;TYPE=WORK:work@example.com" in vcard
+
+
+class TestNormalizeContactData:
+    """Direct tests for the alias helper — it's load-bearing for update_contact too."""
+
+    def test_phone_maps_to_tel(self):
+        assert _normalize_contact_data({"phone": "123"}) == {"tel": "123"}
+
+    def test_organization_maps_to_org(self):
+        assert _normalize_contact_data({"organization": "Acme"}) == {"org": "Acme"}
+
+    def test_canonical_wins_when_both_present(self):
+        """Caller intent: they set ``tel`` deliberately. A stray ``phone`` entry
+        must not clobber the canonical value.
+        """
+        out = _normalize_contact_data({"tel": "canonical", "phone": "alias"})
+        assert out == {"tel": "canonical"}
+
+    def test_does_not_mutate_input(self):
+        original = {"phone": "123", "organization": "Acme"}
+        _normalize_contact_data(original)
+        assert original == {"phone": "123", "organization": "Acme"}
+
+    def test_passthrough_for_unknown_keys(self):
+        assert _normalize_contact_data({"foo": "bar"}) == {"foo": "bar"}