fix(contacts): persist all documented fields on create (fixes #716)

[cbcoutinho]

Summary

• Fixes #716 — nc_contacts_create was silently dropping EMAIL, TEL, ORG, NOTE (and TITLE, NICKNAME, BDAY, CATEGORIES, URL) on contact creation.
• Root cause: create_contact in client/contacts.py only read fn/email/tel from contact_data; every other key was ignored. The reporter's use of phone and organization — valid aliases in intent — weren't even recognized for the two nominally-supported fields, so their entire payload collapsed to just FN + EMAIL.
• Added a shared _build_contact_from_data helper that maps all documented keys to pythonvCard4's Contact constructor. Normalises str → list where the library would otherwise iterate bare strings character-by-character for list-typed properties (e.g. ORG:A;c;m;e given org="Acme"). The same helper is now used by update_contact's fallback branch so create/update stay consistent.
• Expanded the nc_contacts_create_contact tool docstring to enumerate every supported key.

Reproduction (pre-fix)

Given the reporter's call shape:

contact_data = {"fn": "Repro User", "email": "repro@example.com",
                "phone": "555-0716", "organization": "Acme Corp",
                "note": "Issue 716 repro"}

The server persisted only:

BEGIN:VCARD
VERSION:3.0
FN:Repro User
EMAIL;TYPE=HOME:repro@example.com
UID:issue716-repro
END:VCARD

Verification (post-fix)

Same payload, verified end-to-end via stdio MCP:

BEGIN:VCARD
VERSION:3.0
FN:Verified User
EMAIL;TYPE=HOME:verified@example.com
TEL;TYPE=HOME:555-0716
ORG:Acme Corp
TITLE:Engineer
UID:issue716-verify
NOTE:Issue 716 should now be fixed
END:VCARD

Test plan

• uv run pytest tests/client/contacts/ -v (new regression test inspects raw vCard via _get_raw_vcard)
• uv run pytest tests/server/test_contacts_mcp.py -v (MCP workflow extended to assert ORG: / NOTE: round-trip through addressdata)
• uv run pytest tests/unit/ — 537 passed
• uv run ruff check && uv run ruff format — clean
• uv run ty check -- nextcloud_mcp_server — clean
• Live end-to-end via the single-user stdio MCP server against local docker compose stack

---

This PR was generated with the help of AI, and reviewed by a Human

[claude]

Code Review

Good fix for issue #716 — the root cause analysis is accurate and the _build_contact_from_data helper is the right architectural move. The _as_list inner function correctly addresses the pythonvCard4 char-by-char iteration bug. A few things to address:

---

Must fix

1. Missing type annotation on _wrap_contact_field

Per the project's CLAUDE.md convention, all function signatures must be typed. value is untyped:

# current
def _wrap_contact_field(value) -> list[dict]:

# should be
def _wrap_contact_field(value: str | dict | list | None) -> list[dict]:

---

2. Comma-in-org-name bug

_as_list splits on commas for all list-typed fields. For org, this means "Smith, Jones & Associates" becomes ["Smith", "Jones & Associates"], producing ORG:Smith;Jones & Associates — a two-component ORG field per RFC 6350 semantics (company;department), not a single company name.

Comma-splitting makes sense for categories (comma-separated is the documented input format), but not for org, nickname, or url. These should only be list-wrapped without splitting:

def _as_list_no_split(value):
    return value if isinstance(value, list) else [value]

# Use _as_list (with comma-split) only for categories
# Use _as_list_no_split for org, nickname, url

---

3. _merge_vcard_properties (primary update path) silently drops url

url is now in _SUPPORTED_CONTACT_KEYS and documented in the tool, but _merge_vcard_properties — used in the primary update_contact path when an existing vCard is fetched — doesn't handle it at all. After this PR, create_contact persists URLs but update_contact silently drops them (the _build_contact_from_data fix only lands in the fallback branch).

The same gap exists for the phone/organization aliases in _merge_vcard_properties's "add new properties" section (lines ~488–513): it checks for tel and org by exact key, so callers using the alias keys get silent drops on update.

---

Should address

4. _wrap_contact_field docstring mentions URL but the function isn't used for it

The docstring says "Normalize an email/tel/url input" but url goes through _as_list in _build_contact_from_data, not _wrap_contact_field. The docstring is misleading — either update it or route url through _wrap_contact_field (if the type dict shape is needed for URL properties).

---

5. No unit tests for _build_contact_from_data itself

The integration tests added are good, but they go through the full HTTP stack. Pure unit tests would be faster and catch edge cases independently:

• org = "Smith, Jones & Associates" (comma-in-org-name regression once Clean up ci #2 is fixed)
• bday = "not-a-date" (confirm warning logged, no crash)
• Both tel and phone provided simultaneously (which wins?)
• fn=None (should pythonvCard4 allow this?)

---

Nit

# type: ignore[arg-type] on line 113 is understandable given the dynamic kwargs pattern — worth a brief comment explaining why it's needed (e.g., # kwargs built dynamically; Contact typeshed doesn't accept **dict) so it doesn't look like a sloppy suppress.

---

Overall the approach is correct and the tests validate the happy path well. The org-comma bug (#2) and the update-path inconsistency (#3) are the ones most likely to surface in real usage.

[cbcoutinho]

Thanks for the review — addressing each item in a follow-up commit (not yet pushed).

Per-item response

1. Missing type annotation on _wrap_contact_field — fixed. Signature is now def _wrap_contact_field(value: str | dict | list | None) -> list[dict]:.

2. Comma-in-org-name bug — fixed. Split the single inner _as_list into two module-level helpers:

• _as_str_list — wraps str → [str], no comma split. Used for org, nickname, url.
• _split_categories — comma-split for categories only (that's the documented CATEGORIES input shape).

Live-verified via MCP: organization="Smith, Jones & Associates" now persists as ORG:Smith\, Jones & Associates — single component with RFC 6350 comma escaping, not a two-component ORG:Smith;Jones.... Added a unit test (test_org_preserves_comma_in_company_name) that asserts the ORG payload contains no unescaped semicolon separator.

3. _merge_vcard_properties silently drops url + doesn't honour phone alias — fixed. Two parts:

• Added a shared _normalize_contact_data helper that maps phone → tel and organization → org (canonical wins if both are set). Called at the top of both update_contact and _build_contact_from_data, so the merge function now only ever sees canonical keys.
• Added URL handlers in both branches of _merge_vcard_properties (update-existing line and add-new section).

Live-verified: update_contact with {"url": "...", "phone": "555-UPDATE"} now produces a vCard containing both URL: and TEL:555-UPDATE, and the pre-existing ORG: line is preserved. Added a regression step in test_mcp_contacts_workflow asserting the update-with-url path end-to-end.

Nit on claim 3c: _merge_vcard_properties did already handle organization in the add-new branch (line 510, elif key in ["org", "organization"]). The normalisation change in the fix makes this moot anyway — the merge function now never sees organization at all.

4. _wrap_contact_field docstring — fixed. Dropped the stale "url" mention; the docstring now says "email/tel" only, matching the call sites.

5. Missing pure unit tests for _build_contact_from_data — added tests/unit/client/test_contacts.py with 20 cases covering:

• Issue contacts_create — EMAIL, TEL, ORG fields not saved via MCP tool #716 minimal payload round-trip
• Comma-in-org regression (claim Clean up ci #2)
• Invalid bday → warns, no crash
• tel + phone both supplied → tel wins (documents precedence)
• categories string vs. list behaviour (no double-splitting)
• nickname / url bare-string → no char-iteration
• Unknown keys ignored without error
• Plus direct tests for _normalize_contact_data (aliases, canonical precedence, non-mutation)

6. Unannotated # type: ignore[arg-type] — fixed. Now reads # type: ignore[arg-type] # kwargs built dynamically; pythonvCard4's Contact typeshed has specific typed params and doesn't accept **dict[str, Any].

Test matrix

• uv run pytest tests/unit/ — 566 passed
• uv run pytest tests/unit/client/test_contacts.py — 20 passed (new)
• uv run pytest tests/client/contacts/ — 5 passed
• uv run pytest tests/server/test_contacts_mcp.py — 1 passed (now covers the update-with-url path)
• uv run ruff check && uv run ruff format && uv run ty check -- nextcloud_mcp_server — clean

Pushing the follow-up commit shortly.

[claude]

PR Review: fix(contacts): persist all documented fields on create (fixes #716)

Nice fix for a real silent-data-loss bug. The root cause is clearly identified and the helper decomposition is clean. Some observations below — a mix of required fixes and things to consider.

---

Bug: kwargs: dict should be kwargs: dict[str, Any]

Per CLAUDE.md conventions: "Use lowercase generics: dict[str, Any] instead of Dict[str, Any]" — but bare dict gives no info about value types and ty may miss issues inside it.

# contacts.py, _build_contact_from_data
kwargs: dict = {"fn": data.get("fn"), "uid": uid}
# should be:
kwargs: dict[str, Any] = {"fn": data.get("fn"), "uid": uid}

---

Nit: _SUPPORTED_CONTACT_KEYS includes aliases that are never present post-normalization

phone and organization are in the frozenset, but _build_contact_from_data calls _normalize_contact_data first, so by the time the unknown-key check runs those keys no longer exist in data. The frozenset should either contain only canonical keys, or the unknown-key check should run against the pre-normalized input. As-is the comment "Aliases are normalised … before lookup" is slightly contradicted by including them in the set.

_SUPPORTED_CONTACT_KEYS = frozenset({
    "fn", "email", "tel", "org", "note",
    "title", "nickname", "bday", "categories", "url",
})

---

Gap: NICKNAME, BDAY, CATEGORIES are not updatable via _merge_vcard_properties

The PR correctly adds these three fields to the create path (_build_contact_from_data), and the fallback branch of update_contact now uses that helper too — great. But the primary update path (_merge_vcard_properties) has no handling for NICKNAME, BDAY, or CATEGORIES:

• If the existing vCard already has NICKNAME: and the caller passes a new nickname in contact_data, the merge will silently preserve the old value (the catch-all "keep unchanged" clause fires).
• If the existing vCard has no NICKNAME: line, there's no code in the "new fields" section at the bottom of _merge_vcard_properties to append one.

This means the round-trip is broken for update: create with nickname="Bob" works, but then calling update_contact with nickname="Robert" silently leaves NICKNAME:Bob.

This doesn't have to be fixed in this PR if the intent is a minimal targeted fix, but it's worth either:

1. Adding NICKNAME/BDAY/CATEGORIES handlers to _merge_vcard_properties (mirrors how ORG, NOTE, TITLE, URL are handled), or
2. Opening a follow-up issue so this gap is tracked.

---

Minor: URL update only preserves the first item in a list

In _merge_vcard_properties, when url is a list the code takes only [0]:

url_value = url_value[0] if url_value else ""

The create path (_as_str_list) wraps all items. This is a minor inconsistency — contacts rarely have multiple URLs, but worth aligning the two paths if you're adding URL handling to update anyway.

---

Positive notes

• Alias normalization strategy (canonical wins, alias dropped) is exactly the right call — no ambiguity.
• _as_str_list vs _split_categories distinction is clearly documented and correct. The ORG comma-preservation test (test_org_preserves_comma_in_company_name) is exactly the kind of edge-case test this needed.
• _wrap_contact_field handles the str → [{value, type}] promotion cleanly; the dict-with-custom-type test is a good coverage addition.
• # type: ignore[arg-type] on Contact(**kwargs) is the right scalpel — narrow, justified, and explained.
• Unit test suite is comprehensive — 18 focused tests that would have caught the original bug immediately.
• The fallback branch of update_contact now uses _build_contact_from_data, keeping create/update consistent.

Overall this is a solid, well-scoped fix. The main thing I'd want addressed before merge is the NICKNAME/BDAY/CATEGORIES update gap — either in-PR or tracked as a follow-up issue.

[cbcoutinho]

Thanks for the second pass. Responding to the NICKNAME/BDAY/CATEGORIES gap claim specifically — the rest of the items (dict[str, Any], the alias set) are noted and will follow in a separate commit.

The gap claim is inaccurate

If the existing vCard already has NICKNAME: and the caller passes a new nickname in contact_data, the merge will silently preserve the old value (the catch-all "keep unchanged" clause fires).

_merge_vcard_properties does handle all three fields in both branches today (added before this PR, unchanged by it):

• Update-existing branch (contacts.py ~line 457–471): NICKNAME, BDAY, CATEGORIES each have their own elif property_name == "X" and "x" in contact_data: handler, so the catch-all else: updated_lines.append(line) does not fire for them.
• Add-new branch (~line 498–509): same three keys each have their own elif key == "X": handler to append a fresh line when none existed.

Empirically confirmed against the current code:

>>> existing = '…\nNICKNAME:Bob\nBDAY:1990-05-01\nCATEGORIES:friends,work\n…'
>>> _merge_vcard_properties(existing, {"nickname": "Robert",
...                                    "bday": "1991-06-02",
...                                    "categories": ["vip", "new"]}, uid="…")
BEGIN:VCARD
...
NICKNAME:Robert      ← was Bob
BDAY:1991-06-02      ← was 1990-05-01
CATEGORIES:vip,new   ← was friends,work
END:VCARD

And with no existing NICKNAME/BDAY/CATEGORIES lines, the add-new branch correctly appends them.

Pinning this with tests

To turn this into a durable guard rather than a reply, I added a new TestMergeVcardProperties class to tests/unit/client/test_contacts.py with seven direct unit tests against _merge_vcard_properties:

• test_nickname_overwrites_existing_line
• test_bday_overwrites_existing_line
• test_categories_overwrites_existing_line
• test_nickname_added_when_not_in_existing_vcard
• test_bday_added_when_not_in_existing_vcard
• test_categories_added_when_not_in_existing_vcard
• test_url_update_preserves_unrelated_properties

All 7 pass against HEAD of this branch (27/27 total in the file). If any of these ever fail, the gap-claim becomes real and we'll catch it immediately.

Adjacent real concerns

Your general suspicion that create and update can drift is valid — it's just not manifesting as a gap for these three keys today. Two smaller divergences are worth noting for follow-up (not blocking this PR):

1. NICKNAME list format drift. Create (via pythonvCard4) writes list-valued NICKNAME as NICKNAME:Bob;Robbie (semicolons, structured). Update writes it as NICKNAME:Bob,Robbie (commas, joined). Rare shape, but worth aligning.
2. Update BDAY is a raw pass-through. Create validates "not-a-date" and drops it with a warning; update writes it verbatim. Could lift the same date.fromisoformat check up.

Happy to open a follow-up issue for those rather than expand this PR.

[claude]

PR Review — fix/contacts-create-dropped-fields-716

Good fix overall. The root cause is well-diagnosed, the helper decomposition is clean, and the test suite is thorough. A few observations below, roughly ordered by importance.

---

Bug / Correctness

Double normalization in update_contact fallback path

update_contact normalises aliases at the top (contact_data = _normalize_contact_data(contact_data)), then passes the result to _build_contact_from_data, which calls _normalize_contact_data again internally. It's idempotent, so not a bug in practice, but it's slightly confusing and worth aligning. Either:

• Remove the call inside _build_contact_from_data and require callers to normalize first, or
• Remove the call at the top of update_contact and rely on _build_contact_from_data to handle it.

The simplest fix is to remove the double call from _build_contact_from_data since update_contact already normalizes before calling it (or document the invariant in a comment).

---

Code Quality

_SUPPORTED_CONTACT_KEYS includes aliases that never appear in the normalized dict

The unknown-key check compares against the normalized data:

data = _normalize_contact_data(contact_data)
# ...
unknown = set(data) - _SUPPORTED_CONTACT_KEYS

After normalization, phone → tel and organization → org, so data will never contain phone or organization. Including those aliases in _SUPPORTED_CONTACT_KEYS is harmless but misleading — the set comment says "Canonical keys … Aliases are normalised before lookup", yet the comparison runs on the already-normalized dict. Consider either:

• Removing "phone" and "organization" from the set (and updating the comment), or
• Running the unknown-key check against contact_data (before normalization) if you want to warn about truly unrecognised keys that aren't aliases either.

Untyped generics in helper signatures

Per the project's CLAUDE.md conventions (dict[str, Any] not bare dict, typed generics):

# Current
def _wrap_contact_field(value: str | dict | list | None) -> list[dict]:
def _as_str_list(value: str | list) -> list[str]:
def _split_categories(value: str | list) -> list[str]:
kwargs: dict = ...

# Preferred
def _wrap_contact_field(value: str | dict[str, Any] | list[str | dict[str, Any]] | None) -> list[dict[str, Any]]:
def _as_str_list(value: str | list[str]) -> list[str]:
def _split_categories(value: str | list[str]) -> list[str]:
kwargs: dict[str, Any] = ...

---

Minor Observations (no action required)

Multi-URL merge only keeps the first URL in the list

In both the existing-property and new-property branches of _merge_vcard_properties, only url_value[0] is used when url_value is a list. For most use cases this is fine, but a comment would prevent future confusion:

# Only the first URL is written; multiple-URL contacts are uncommon and
# the vCard-merge parser doesn't attempt to map list positions to existing lines.
url_value = value[0] if isinstance(value, list) and value else value

fn is required in the docstring but handled as optional internally

kwargs = {"fn": data.get("fn"), ...} silently passes fn=None to pythonvCard4 when fn is omitted. pythonvCard4 may or may not tolerate this. Consider a guard:

fn = data.get("fn")
if not fn:
    raise ValueError("contact_data must include 'fn' (formatted full name)")

…or at minimum a warning log.

---

Test Coverage

The test suite is excellent — unit tests cover alias precedence, char-iteration regression, bday edge cases, empty values, and unknown keys. The TestMergeVcardProperties class is particularly useful as living documentation of the merge function's contract.

One small gap: _wrap_contact_field with a dict that has no value key (e.g. {"type": ["WORK"]}) hits item.get("value") → falsy and is silently dropped. That's reasonable behaviour, but a quick test asserting the drop would document the intent.

---

Summary

The fix is correct and the regression test is solid. The main actionable items are:

1. Resolve the double-normalization in the update_contact fallback path.
2. Clean up _SUPPORTED_CONTACT_KEYS to avoid the misleading alias inclusion.
3. Add specific generic types to the helper signatures (ty/ruff will catch these too).

Happy to approve after (1) and (2) are addressed; (3) is cosmetic.

[CLAassistant]

All committers have signed the CLA.

[claude]

Review: fix(contacts): persist all documented fields on create

Good fix for a real user-facing bug. The _build_contact_from_data helper is clean, the alias normalization logic is correct, and the test coverage is thorough. A few things worth addressing:

---

Dead code in _merge_vcard_properties

_normalize_contact_data is now called before _merge_vcard_properties in update_contact, so "organization" will never appear in contact_data at that point. Two dead code paths remain:

In the update-existing branch:

elif property_name == "ORG" and (
    "org" in contact_data or "organization" in contact_data  # "organization" unreachable
):
    org_value = contact_data.get("org") or contact_data.get("organization")  # second .get() unreachable

In the add-new branch:

elif key in ["org", "organization"]:  # "organization" unreachable

These should be simplified to just check "org". No functional impact, but it is a trap for future readers.

---

Silent drop of list/dict email/tel when updating a contact without an existing EMAIL/TEL line

_wrap_contact_field correctly handles str or dict or list for create, but the _merge_vcard_properties add-new section only handles str:

elif key == "email" and isinstance(value, str):
    updated_lines.append(f"EMAIL:{value}")
elif key == "tel" and isinstance(value, str):
    updated_lines.append(f"TEL:{value}")

If an update passes {"email": [{"value": "work@example.com", "type": ["WORK"]}]} to a contact with no existing EMAIL line, the value is silently dropped. This is pre-existing, but now that _wrap_contact_field exists and is used on the create path, applying it here would close the gap. At minimum the limitation should be noted in a comment.

---

Missing @pytest.mark.integration on the new regression test

test_create_contact_persists_all_documented_fields in tests/client/contacts/test_contacts_operations.py hits real Nextcloud via nc_client but lacks the mark. Other tests in the same file carry @pytest.mark.integration. Without it the test is excluded from "pytest -m integration" runs but included in the default suite - opposite of intended.

---

BDAY in _merge_vcard_properties does not validate like _build_contact_from_data does

_build_contact_from_data guards against invalid ISO strings and logs a warning. The merge path writes the value verbatim. A date object works fine (str(date(...)) is ISO), but an invalid string like "not-a-date" would be written to the vCard without warning. Minor inconsistency worth aligning.

---

Minor observation on TestMergeVcardProperties

The test class correctly documents existing behavior for NICKNAME/BDAY/CATEGORIES, but those handlers were already present in _merge_vcard_properties before this PR (the PR only added URL). The tests are useful regression coverage - just worth noting they pin pre-existing behavior, not new behavior added by this PR.

---

Overall

The core fix is correct and well-structured. The alias normalization returning a new dict (not mutating), the _as_str_list wrapper preventing pythonvCard4 char-iteration, and the ISO date validation for BDAY are all solid. The dead-code cleanup and missing test mark are the most actionable items before merge.

[claude]

---

Code Review

Good fix overall. The root cause is correctly identified, the central _build_contact_from_data helper and alias normalisation is a clean design, and test coverage is thorough (unit + integration + smoke).

A few things worth addressing before merging:

---

Bug: list-form email/tel silently drops the existing line in _merge_vcard_properties

The PR enables dict/list email inputs via _wrap_contact_field for create (good), but the update path has a pre-existing gap that becomes more reachable now (client/contacts.py around line 482):

elif property_name == "EMAIL" and "email" in contact_data:
    if "email" not in updated_properties:
        if isinstance(contact_data["email"], str):   # only handles str
            updated_lines.append(...)
        updated_properties.add("email")  # marks done even if nothing appended
    else:
        updated_lines.append(line)

If a caller passes email as a dict (e.g. dict(value="work@example.com", type=["WORK"])) or a list, the existing EMAIL: line is consumed from the loop but nothing is written to updated_lines, silently deleting the contact's email. Same issue exists for tel.

Neither block was touched by this PR, but the PR now documents and accepts these richer forms for create — so an update roundtrip will silently drop the email. Suggested fix: add an else branch that falls through to updated_lines.append(line) (keep original) when the input type is not handled.

---

Dead code in _merge_vcard_properties

After _normalize_contact_data runs in update_contact, the "organization" key is always gone. These two checks are now unreachable:

Update-existing branch (~line 532):

elif property_name == "ORG" and (
    "org" in contact_data or "organization" in contact_data  # "organization" is dead
):
    org_value = contact_data.get("org") or contact_data.get("organization")  # second get() always None

Add-new section (~line 583):

elif key in ["org", "organization"]:  # "organization" is dead

Both "organization" references can be dropped.

---

BDAY validation inconsistency between create and update paths

_build_contact_from_data validates the ISO string and drops it with a warning on failure. But _merge_vcard_properties writes it raw in the add-new section (~line 577):

elif key == "bday":
    updated_lines.append(f"BDAY:{value}")   # no ISO validation

A non-ISO BDAY string on update produces a malformed vCard. Worth extracting the parse step into a small helper or into _normalize_contact_data so both paths share it.

---

Minor: vCard property injection via newlines (pre-existing, noting because the PR widens the surface)

_merge_vcard_properties interpolates values directly, e.g. f"NOTE:{contact_data['note']}". A value containing a literal newline would inject additional vCard properties. This is pre-existing, but since this PR extends the set of fields going through this path (url, title, etc.) it is worth adding sanitisation — at minimum replace newlines with the vCard folding escape before interpolation.

---

Nit

_merge_vcard_properties is missing a -> str return type annotation while the rest of the file is fully annotated.

---

Summary: The core fix is correct and the helper factoring is clean. The list-email/tel silent-drop issue in _merge_vcard_properties is the one I would ask to address before merging, since the PR now makes that code path reachable through its own documented API surface.

[claude]

Code Review: PR #719 — fix(contacts): persist all documented fields on create

Good fix for a real user-visible bug. The overall approach is sound: extracting _build_contact_from_data as a shared helper and applying _safe_vcard_value across the raw-text merge path are both the right calls. The test suite is thorough. A few things to address:

---

Bug: list-form org mangled in _merge_vcard_properties

_safe_vcard_value only handles str — it passes non-strings through unchanged. But contact_data['org'] can legitimately be a list (e.g. ["Acme", "Engineering"]) since _build_contact_from_data accepts that shape. When it reaches both branches in _merge_vcard_properties that emit the ORG line, they produce Python repr instead of a valid vCard line.

The correct vCard serialisation is ORG:Acme;Engineering (semicolons between components per RFC 6350). The fix for both the update-existing and add-new-field branches: check isinstance(org_value, list) and join with ; before passing through _safe_vcard_value.

The _build_contact_from_data path is unaffected — pythonvCard4 handles serialisation there.

---

Bug: bare-string type in _wrap_contact_field is character-iterated

The function does:

types = item.get("type") or ["HOME"]
out.append(...)   # appends dict with "value" and "type": list(types)

If a caller passes a dict with "type": "WORK" (bare string instead of list), list("WORK") yields ["W", "O", "R", "K"]. A guard for isinstance(types, str) is needed before the list() call.

---

Undocumented silent-drop: dict/list email or tel on update

In _merge_vcard_properties, when contact_data["email"] is a dict or list, the code preserves the existing line but does NOT add "email" to updated_properties. The "add new fields" pass also skips it (it has an isinstance(value, str) guard). Net result: the update is silently discarded — no error, no log.

This is a defensible safety choice, but callers who pass the create-style dict form into update_contact get a silent no-op. At minimum this should be logged at debug level and/or documented in the nc_contacts_update_contact tool docstring. The test test_dict_email_input_preserves_existing_line already pins the behaviour, just not the UX gap.

---

Test gap: list-form org through _merge_vcard_properties

TestMergeVcardProperties has no case for org=["Acme", "Engineering"]. Adding one would have caught the ORG formatting bug above and pins the expected ORG:Acme;Engineering output.

---

Overall

The core fix is correct and well-structured. Alias normalisation is centralised, the helper extraction is clean, and the injection-hardening via _safe_vcard_value (plus test_newline_in_note_does_not_inject_property) is a nice addition. Fix the two bugs above and document the dict/list email behaviour and this is production-ready.

[claude]

Code Review — Fix contacts: persist all documented fields on create (#719)

Good overall fix. The root cause is correctly identified and the _build_contact_from_data helper is a clean solution. Test coverage is thorough. Here are my findings:

---

Issues

1. Silent no-op when updating email/tel with dict/list input on a contact that has no existing email/tel

In _merge_vcard_properties, the add-new section (after the line-scan loop) only handles string-form email and tel:

elif key == "email" and isinstance(value, str):
    updated_lines.append(f"EMAIL:{_safe_vcard_value(value)}")
elif key == "tel" and isinstance(value, str):
    updated_lines.append(f"TEL:{_safe_vcard_value(value)}")

Meanwhile _build_contact_from_data (used by create_contact) fully supports dict/list via _wrap_contact_field. So this workflow silently does nothing:

# create a contact with no email
await create_contact(uid=uid, contact_data={"fn": "Alice"})

# try to add email via update — dict input, contact has no existing EMAIL line
await update_contact(uid=uid, contact_data={"email": {"value": "alice@work.com", "type": "WORK"}})
# -> EMAIL is never written. No error. No warning.

The unit test test_dict_email_input_preserves_existing_line only covers the case where an EMAIL line already exists; the "no existing EMAIL + dict input = nothing added" path is not tested.

Suggestion: Either (a) fall back to _wrap_contact_field in the add-new branch for dict/list inputs, or (b) log a warning so the silent drop is at least observable.

---

2. _merge_vcard_properties dict/list email case doesn't mark updated_properties

In the update-existing branch for EMAIL:

if isinstance(contact_data["email"], str):
    ...
    updated_properties.add("email")
else:
    # keep original line
    updated_lines.append(line)
    # NOTE: updated_properties is NOT updated here

Because updated_properties is never set when the input is a dict, a contact with multiple EMAIL lines will have every one of them reach the "email" not in updated_properties branch, appending each in turn — which is arguably correct (preserve all). But then the add-new loop also sees key not in updated_properties, hits the isinstance(value, str) guard, and silently does nothing. The asymmetry between the two paths is at least internally consistent but the interaction is subtle enough that it warrants a comment at the else: branch:

else:
    # Non-str inputs are not supported by the text-merge path;
    # keep the existing line unchanged and do NOT set updated_properties
    # so additional EMAIL lines also pass through (see add-new loop below
    # which guards on isinstance(value, str) for the same reason).
    updated_lines.append(line)

---

3. nc_contacts_update_contact docstring not updated

nc_contacts_create_contact in server/contacts.py now documents all supported keys and their accepted types. nc_contacts_update_contact's docstring for contact_data was not touched, so callers still see the old one-liner. Given that the two tools accept the same contact_data shape, a consistent docstring (or at least a note that dict/list email/tel are only partially supported on update) would help.

---

4. _safe_vcard_value() prevents newline injection but not backslash injection

The docstring cites RFC 6350 §3.4, which also requires escaping \ → \\ in property values. A value like "notes\\" would be written literally, and a vCard reader would interpret it as an escaped sequence. This is a minor correctness gap (not a real injection vector), but for full RFC 6350 compliance in the manual-merge path, backslash escaping is missing. Consider extending the helper or adding a comment explaining the deliberate scope limitation.

---

Minor

• The _SUPPORTED_CONTACT_KEYS frozenset is only referenced in _build_contact_from_data. The add-new section of _merge_vcard_properties duplicates the same key list as a chain of elif key == branches. Not a bug, but if a new field is added to _SUPPORTED_CONTACT_KEYS in future, this second list would need to be updated independently. Worth a # keep in sync with _SUPPORTED_CONTACT_KEYS comment.

• The new integration test test_create_contact_persists_all_documented_fields in tests/client/contacts/test_contacts_operations.py doesn't carry an explicit @pytest.mark.integration marker. The existing test in the same file (test_full_contact_workflow) appears to have the same omission, so this may be handled by a directory-level marker, but it's worth confirming.

---

What's good

• The fix correctly targets the root cause (manual field-by-field assignment replaced by a single helper with comprehensive mapping).
• _safe_vcard_value() is a genuine security improvement: newline injection in vCard property values is a real concern and the test at test_newline_in_note_does_not_inject_property pins it.
• The _normalize_contact_data alias normalisation (phone→tel, organization→org) is now shared between create and update, preventing the update path from re-introducing the same drift.
• RFC 6350 §6.6.4 compliance for ORG (semicolon-separated components vs. comma-split) is correctly handled and tested.
• The _wrap_contact_field char-iteration footgun fix (bare string "WORK" → ["W","O","R","K"]) is well-explained and tested.
• Unit test coverage in tests/unit/client/test_contacts.py is comprehensive — 370 lines covering happy paths, aliases, edge cases, security, and the merge path. The TestMergeVcardProperties class in particular pins behaviours that were previously only observable end-to-end.

[claude]

Code Review: PR 719 final pass

This PR has gone through thorough iterative review and the code has materially improved with each commit. The core fix is correct, the helper decomposition is clean, and the test suite (411 lines of targeted unit tests + integration coverage) is exemplary. Here is where things stand after the last commit.

Items addressed: All major issues from previous rounds are resolved: type annotations, comma-in-org-name, BDAY validation parity, vCard injection via _safe_vcard_value, dead-code cleanup (organization alias in _merge_vcard_properties), list-form ORG serialisation, bare-string type char-iteration in _wrap_contact_field, dict/list email/tel warning + docstring, and the _SUPPORTED_CONTACT_KEYS alias cleanup. All eight commits tell a clear story.

---

Remaining items

1. Warning message is imprecise when the contact has no existing EMAIL/TEL line

The warning fires before the line-scan loop and says "existing EMAIL lines are preserved unchanged". If the contact has no EMAIL line, this is technically true but the more actionable fact -- that the dict/list email will not be added either -- is not stated. The test test_dict_email_on_no_existing_line_warns verifies the warning fires but the message text does not surface that EMAIL will be absent from the result.

Adding "and no new EMAIL lines are added" to the warning text would close the gap. Minor, does not block merge.

2. _safe_vcard_value scope: RFC 6350 also requires backslash escaping

RFC 6350 section 3.4 requires escaping backslashes in TEXT values in addition to newlines. The injection risk from an unescaped backslash is low (it is not a line terminator), but for correctness the helper could escape backslash first then escape newlines. Alternatively a docstring note clarifying the intentional scope (injection prevention only; full TEXT escaping deferred to pythonvCard4 on the create path) would be sufficient.

3. Missing @pytest.mark.integration on new regression test

test_create_contact_persists_all_documented_fields in tests/client/contacts/test_contacts_operations.py uses nc_client (real Nextcloud) but has no marker decorator. If a directory-level conftest applies the marker automatically this is fine. Otherwise pytest -m "not integration" will try to run it against a live stack and fail in environments without containers. Worth confirming, and adding pytestmark = pytest.mark.integration at module level if needed -- consistent with how tests/unit/client/test_contacts.py uses pytestmark = pytest.mark.unit.

---

Overall: Solid, well-scoped fix for a real silent data-loss bug. The test pyramid (unit + client integration + MCP workflow smoke) is exactly the right shape. Items 1 and 3 are most actionable before merge; item 2 is a minor RFC compliance/documentation note. Happy to approve once the integration marker situation is confirmed.