Add Contacts tools with CardDAV support#37
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a CardDAV-based Contacts MCP toolset (six tools) with vCard parsing and ETag-aware write operations, registers it in the server, seeds contact test data, updates Pyright and progress docs, and adds comprehensive integration tests and test-server expectations. (29 words) Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant MCP as FastMCP / contacts tool
participant DAV as Nextcloud CardDAV
Client->>MCP: create_contact(payload)
MCP->>DAV: PUT /addressbooks/.../{uid}.vcf (Content-Type: text/vcard)
DAV-->>MCP: 201 Created + ETag
MCP->>DAV: REPORT (retrieve stored vCard + etag)
DAV-->>MCP: vCard + ETag
MCP-->>Client: contact JSON + etag
Client->>MCP: get_contacts(book_id, limit, offset)
MCP->>DAV: REPORT (query contacts)
DAV-->>MCP: list of hrefs + vCards + etags
MCP-->>Client: paginated contact list (has_more, items)
Client->>MCP: update_contact(uid, etag, changes)
MCP->>DAV: REPORT (fetch current vCard)
DAV-->>MCP: vCard + current-etag
MCP->>DAV: PUT updated .vcf with If-Match: etag
DAV-->>MCP: 204 No Content / new ETag
MCP-->>Client: updated contact JSON
Client->>MCP: delete_contact(uid)
MCP->>DAV: REPORT (locate href)
DAV-->>MCP: href
MCP->>DAV: DELETE /addressbooks/.../{uid}.vcf
DAV-->>MCP: 204 No Content
MCP-->>Client: deletion confirmation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #37 +/- ##
==========================================
+ Coverage 95.97% 96.09% +0.12%
==========================================
Files 24 25 +1
Lines 1911 2282 +371
==========================================
+ Hits 1834 2193 +359
- Misses 77 89 +12
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/nc_mcp_server/tools/contacts.py (1)
348-355: Sort before slicing so offset pagination is stable.
get_contactspaginates in raw REPORT order. If the server changes response order between calls, contacts can jump between pages or be skipped/duplicated. Sorting by a stable key like UID orhrefbefore applyingoffset/limitmakes the pagination contract deterministic.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nc_mcp_server/tools/contacts.py` around lines 348 - 355, The pagination is unstable because results are sliced in REPORT order; modify the loop in get_contacts (the block using results = _parse_report_xml(...) and calling _format_contact) to attach a stable sort key (e.g., set contact["href"] = _href and preserve contact["uid"] if available from _format_contact) and then sort all_contacts by that stable key (prefer contact.get("uid") with a fallback to contact["href"]) before computing page = all_contacts[offset:offset+limit] and has_more. This ensures deterministic ordering regardless of server response order.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/nc_mcp_server/tools/contacts.py`:
- Around line 471-483: The update_contact function currently uses empty-string
defaults and filters updates by truthiness, preventing clearing fields; change
optional parameters (full_name, given_name, family_name, email, phone,
organization, title, note, book_id) to default to None, build the updates dict
using "is not None" for each parameter (so None means "not provided"), and
update _apply_contact_updates to treat an explicit empty string as a removal
(strip the field and do not reinsert it when the new value is empty); apply the
same None-default + "is not None" pattern to the other similar contact-update
function(s) in this file.
- Around line 202-232: The _build_vcard function is incorrectly using xml_escape
for vCard text causing characters like "&" and newlines to be mangled; replace
xml_escape calls in _build_vcard (for UID, FN, N, EMAIL, TEL, ORG, TITLE, NOTE
and any constructed FN/N fields) with a vCard-safe text escaper/serializer
(escape backslashes, semicolons, commas, and newlines per vCard 3.0 rules and
handle line folding) and reuse the same helper used by _apply_contact_updates
(or add a single helper named e.g. vcard_escape_text and call it from both
_build_vcard and _apply_contact_updates) so all contact fields are serialized
consistently. Ensure UID remains unchanged if provided and that the function
still joins lines with "\r\n" and appends the final "\r\n".
- Around line 284-291: The current partial-name update in the block using
ICal.from_ical and card.get("N") overwrites the entire N component as
"family;given;;;" and thus drops existing additional, prefix, and suffix fields;
fix it by reading old_n.fields.additional, old_n.fields.prefix, and
old_n.fields.suffix (falling back to empty strings if old_n is None or missing
attributes) and construct the new N as "family;given;additional;prefix;suffix"
using xml_escape, so new_lines.insert(insert_before, f"N:{...}") preserves any
existing components when only given_name or family_name are updated (keep using
ICal.from_ical, card.get("N"), new_lines.insert, xml_escape and the updates dict
to locate values).
In `@tests/integration/test_contacts.py`:
- Around line 20-29: The autouse fixture _cleanup_test_contacts currently
depends on the shared nc_mcp test helper (which bootstraps module-level
client/config/permission state) and thus makes permission tests order-dependent;
change the fixture to avoid using nc_mcp and instead perform cleanup via a
direct DAV client or the same isolated fixture under test (e.g., replace the
nc_mcp parameter with a raw dav client fixture or an isolated helper) and call
the remote endpoints directly (use the equivalent of get_contacts and
delete_contact via the DAV client) so you don't mutate the shared singleton;
ensure you still filter by uid.startswith("mcp-") and PREFIX in full_name and
suppress ToolError during delete.
---
Nitpick comments:
In `@src/nc_mcp_server/tools/contacts.py`:
- Around line 348-355: The pagination is unstable because results are sliced in
REPORT order; modify the loop in get_contacts (the block using results =
_parse_report_xml(...) and calling _format_contact) to attach a stable sort key
(e.g., set contact["href"] = _href and preserve contact["uid"] if available from
_format_contact) and then sort all_contacts by that stable key (prefer
contact.get("uid") with a fallback to contact["href"]) before computing page =
all_contacts[offset:offset+limit] and has_more. This ensures deterministic
ordering regardless of server response order.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 17a4ec9d-7cb1-4690-9714-09f65e35a379
📒 Files selected for processing (7)
PROGRESS.mdpyproject.tomlscripts/seed_pagination_data.pysrc/nc_mcp_server/server.pysrc/nc_mcp_server/tools/contacts.pytests/integration/test_contacts.pytests/integration/test_server.py
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
src/nc_mcp_server/tools/contacts.py (2)
480-529:⚠️ Potential issue | 🟠 Major
update_contactstill can't clear a populated field.This matches the earlier unresolved concern: every optional arg defaults to
"", andupdateskeeps only truthy values.note="",email="", etc. are therefore indistinguishable from “not provided”, so once a field is set there is no tool path to remove it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nc_mcp_server/tools/contacts.py` around lines 480 - 529, The update_contact function uses empty-string defaults and filters falsy values, so callers cannot clear existing fields; change each optional parameter (full_name, given_name, family_name, email, phone, organization, title, note, and book_id if applicable) to default to None and update the construction of updates to include keys when the value is not None (i.e., differentiate None == not provided vs "" == explicit clear). Update the function signature and any type hints to accept Optional[str], and ensure the rest of update_contact (including the updates dict logic and the ValueError check) treats None as absence and empty string as an explicit value to send to the backend.
268-304:⚠️ Potential issue | 🟠 MajorUnfold logical vCard lines before stripping fields.
This still has the earlier line-folding problem in a different form:
_strip_updated_fields()works on raw physical lines, so updating a foldedNOTE/ORG/ADRremoves only the first line and leaves the continuation behind. When the card is reassembled, that orphaned continuation line gets attached to the previous property and corrupts the contact.Suggested fix
+def _unfold_vcard_lines(vcard_data: str) -> list[str]: + lines: list[str] = [] + for raw_line in vcard_data.splitlines(): + clean = raw_line.rstrip("\r") + if clean[:1] in {" ", "\t"} and lines: + lines[-1] += clean[1:] + else: + lines.append(clean) + return lines + def _apply_contact_updates(vcard_data: str, updates: dict[str, Any]) -> str: """Apply partial field updates to existing vCard data, return new vCard string.""" skip_fields = {field for key, field in _UPDATE_FIELD_MAP if key in updates} if ("given_name" in updates or "family_name" in updates) and "full_name" not in updates: skip_fields.add("FN") - new_lines = _strip_updated_fields(vcard_data.strip().split("\n"), skip_fields) + new_lines = _strip_updated_fields(_unfold_vcard_lines(vcard_data), skip_fields)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/nc_mcp_server/tools/contacts.py` around lines 268 - 304, The bug is that folded vCard logical lines (continuations starting with space/tab) are not unfolded before _strip_updated_fields runs, so only the first physical line is removed and orphaned continuations remain; to fix, unfold logical lines first inside _apply_contact_updates (before calling _strip_updated_fields) or update _strip_updated_fields to accept already-unfolded lines: implement an unfold step that joins any line that begins with a space or tab to the previous line (preserving exact whitespace rules and handling CR/LF), then pass the unfolded lines to _strip_updated_fields (or operate on them in-place), ensuring all mentions of vCard line splitting use this unfolded representation (reference functions: _apply_contact_updates, _strip_updated_fields and keep using _vcard_escape for inserted lines).tests/integration/test_contacts.py (1)
20-21:⚠️ Potential issue | 🟠 MajorThis autouse cleanup fixture still makes the permission tests order-dependent.
_cleanup_test_contacts()bootstrapsnc_mcpbefore every test, while the permission cases also requestnc_mcp_read_only/nc_mcp_write. Because the tools read client/config/permission from module state at call time, whichever fixture initializes last controls the effective permission level, so those assertions are not actually isolated.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_contacts.py` around lines 20 - 21, The autouse fixture _cleanup_test_contacts is initializing nc_mcp for every test and clobbering permission-specific fixtures (nc_mcp_read_only / nc_mcp_write), making tests order-dependent; change the fixture so it does not forcibly create nc_mcp for every test—either remove autouse=True and require tests that need cleanup to explicitly use the fixture, or rewrite _cleanup_test_contacts to accept the pytest request fixture and call request.getfixturevalue to obtain the appropriate helper (e.g., request.getfixturevalue("nc_mcp_read_only") / "nc_mcp_write" / "nc_mcp") before performing cleanup so you never instantiate the wrong nc_mcp variant and preserve isolation for permission tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/nc_mcp_server/tools/contacts.py`:
- Around line 248-265: The current update mapping (_UPDATE_FIELD_MAP and
_SIMPLE_UPDATE_FIELDS) causes all existing EMAIL/TEL vCard properties to be
removed and replaced with a single entry (e.g., "EMAIL;TYPE=WORK" or
"TEL;TYPE=CELL"); change the update logic so that when updating email or phone
you merge with existing properties instead of stripping them: locate the code
that uses _SIMPLE_UPDATE_FIELDS/_UPDATE_FIELD_MAP to write vCard properties and
modify it to (1) read existing EMAIL/TEL entries into a list, (2) update the
intended entry by matching an identifier (type, value, or a “primary” flag) or
append a new entry if no match, and (3) rewrite all EMAIL/TEL entries preserving
their types/metadata; keep the other simple fields behavior the same and ensure
updates of "email" and "phone" operate on arrays rather than replacing the
entire set.
In `@tests/integration/test_contacts.py`:
- Around line 23-28: The cleanup currently checks PREFIX in
contact.get("full_name", "") which is mutable and causes test-created mcp-*
contacts to be missed; change the condition to rely only on the immutable UID
prefix (e.g., uid.startswith("mcp-")) when deciding to delete contacts. In the
loop that calls nc_mcp.call("get_contacts", ...) and later
nc_mcp.call("delete_contact", ...), remove the PREFIX/full_name check and delete
any contact where contact["uid"].startswith("mcp-") (optionally also verify
BOOK_ID if needed) so cleanup no longer depends on display name mutations.
---
Duplicate comments:
In `@src/nc_mcp_server/tools/contacts.py`:
- Around line 480-529: The update_contact function uses empty-string defaults
and filters falsy values, so callers cannot clear existing fields; change each
optional parameter (full_name, given_name, family_name, email, phone,
organization, title, note, and book_id if applicable) to default to None and
update the construction of updates to include keys when the value is not None
(i.e., differentiate None == not provided vs "" == explicit clear). Update the
function signature and any type hints to accept Optional[str], and ensure the
rest of update_contact (including the updates dict logic and the ValueError
check) treats None as absence and empty string as an explicit value to send to
the backend.
- Around line 268-304: The bug is that folded vCard logical lines (continuations
starting with space/tab) are not unfolded before _strip_updated_fields runs, so
only the first physical line is removed and orphaned continuations remain; to
fix, unfold logical lines first inside _apply_contact_updates (before calling
_strip_updated_fields) or update _strip_updated_fields to accept
already-unfolded lines: implement an unfold step that joins any line that begins
with a space or tab to the previous line (preserving exact whitespace rules and
handling CR/LF), then pass the unfolded lines to _strip_updated_fields (or
operate on them in-place), ensuring all mentions of vCard line splitting use
this unfolded representation (reference functions: _apply_contact_updates,
_strip_updated_fields and keep using _vcard_escape for inserted lines).
In `@tests/integration/test_contacts.py`:
- Around line 20-21: The autouse fixture _cleanup_test_contacts is initializing
nc_mcp for every test and clobbering permission-specific fixtures
(nc_mcp_read_only / nc_mcp_write), making tests order-dependent; change the
fixture so it does not forcibly create nc_mcp for every test—either remove
autouse=True and require tests that need cleanup to explicitly use the fixture,
or rewrite _cleanup_test_contacts to accept the pytest request fixture and call
request.getfixturevalue to obtain the appropriate helper (e.g.,
request.getfixturevalue("nc_mcp_read_only") / "nc_mcp_write" / "nc_mcp") before
performing cleanup so you never instantiate the wrong nc_mcp variant and
preserve isolation for permission tests.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 34c36fb9-6d4e-4939-a9ef-36351a197ae5
📒 Files selected for processing (2)
src/nc_mcp_server/tools/contacts.pytests/integration/test_contacts.py
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
tests/integration/test_contacts.py (1)
633-636:⚠️ Potential issue | 🟠 MajorDrop the extra
nc_mcpfixture from this permission test.Requesting both fixtures mutates the same global server state twice, so this assertion can pass under the wrong permission context. Keeping only
nc_mcp_writepreserves the isolation you just restored elsewhere.🧪 Proposed fix
- async def test_write_allows_create(self, nc_mcp: McpTestHelper, nc_mcp_write: McpTestHelper) -> None: + async def test_write_allows_create(self, nc_mcp_write: McpTestHelper) -> None: result = await nc_mcp_write.call("create_contact", full_name=f"{PREFIX}-write-ok", book_id=BOOK_ID) contact = json.loads(result) assert contact["uid"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/integration/test_contacts.py` around lines 633 - 636, The test test_write_allows_create currently requests both fixtures nc_mcp and nc_mcp_write which mutates shared server state; remove the extra fixture by deleting nc_mcp from the test signature so the function only accepts nc_mcp_write, and keep the rest of the body (the call to nc_mcp_write.call("create_contact", ...), JSON decode and assert) unchanged to ensure the test runs under the intended write-only permission context.src/nc_mcp_server/tools/contacts.py (1)
248-265:⚠️ Potential issue | 🟠 MajorUpdating
phonestill wipes every other value on the card.This path strips all existing
TELlines and then writes back exactly oneWORK/CELLproperty. Any secondary addresses/numbers and their type metadata are lost on a partial update.Also applies to: 295-317
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/nc_mcp_server/tools/contacts.py`:
- Around line 295-315: The name-write path in _apply_contact_updates is keyed on
truthiness so passing full_name="" or clearing given/family can strip FN or
leave an empty FN; change the logic to treat presence in updates (use "if
'full_name' in updates" and check for keys for given_name/family_name) and
ensure a valid FN is always inserted before returning: after computing
family/given/additional/prefix/suffix (using ICal.from_ical and old_n as today),
synthesize a fallback FN when updates result in an empty full name by deriving
one from the new structured parts (e.g., f"{given} {family}".strip()) or, if
those are empty, fall back to the existing FN value from the parsed vcard;
always insert that FN line (using _vcard_escape) rather than skipping when falsy
so the vCard never ends up with a missing or empty FN; update the same logic in
the other occurrence mentioned (around lines 529-542).
- Around line 45-47: _vcard_escape currently only replaces "\n" so inputs with
Windows CRLF or lone CR leave "\r" characters that can break vCard lines; update
the _vcard_escape function to normalize CRLF and CR to plain "\n" first (e.g.,
normalize "\r\n" and "\r" to "\n") and then perform the existing escaping of
backslashes, semicolons, commas and newlines so the returned string contains no
raw "\r" characters that could corrupt multiline properties like NOTE.
---
Duplicate comments:
In `@tests/integration/test_contacts.py`:
- Around line 633-636: The test test_write_allows_create currently requests both
fixtures nc_mcp and nc_mcp_write which mutates shared server state; remove the
extra fixture by deleting nc_mcp from the test signature so the function only
accepts nc_mcp_write, and keep the rest of the body (the call to
nc_mcp_write.call("create_contact", ...), JSON decode and assert) unchanged to
ensure the test runs under the intended write-only permission context.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 432e5a56-3439-4603-a0dc-caf26290333d
📒 Files selected for processing (2)
src/nc_mcp_server/tools/contacts.pytests/integration/test_contacts.py
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/integration/test_contacts.py`:
- Around line 801-802: The test function test_write_allows_create currently
declares two fixtures (nc_mcp and nc_mcp_write) which couples tests via shared
server state; remove the unused nc_mcp fixture from the function signature so
the test only depends on nc_mcp_write, leaving the body calling
nc_mcp_write.call("create_contact", ...). Update the function definition to
accept only nc_mcp_write (preserving its type annotation McpTestHelper) and run
the same assertion to avoid the extra fixture setup.
- Around line 52-54: The cleanup NextcloudClient is not guaranteed to be closed
if _delete_mcp_contacts raises or the task is cancelled; wrap the client usage
in a try/finally so client.close() is always awaited in the finally block.
Locate the client creation (NextcloudClient(_cleanup_config)), the call to
_delete_mcp_contacts(client, _cleanup_config.user), and the await client.close()
and change the flow to create the client, call _delete_mcp_contacts inside try,
and await client.close() inside finally to ensure closure on all failures or
cancellations.
- Around line 33-37: The cleanup loop currently wraps parsing/formatting and
deletion in a single contextlib.suppress(Exception) which lets a failure in
_parse_report_xml or _format_contact (or a missing contact["uid"]) abort the
whole loop; instead, limit the suppression to only the deletion step: call
_parse_report_xml and _format_contact outside the suppress block, validate
contact.get("uid") and compute resource (using BOOK_ID and contact['uid']) and
skip/log the bad contact on failure, then use contextlib.suppress(Exception)
only around the code that performs the remote delete so one bad payload won’t
stop cleaning remaining mcp-* contacts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ea8e52a1-031e-4109-a1ab-03aa89a18096
📒 Files selected for processing (2)
src/nc_mcp_server/tools/contacts.pytests/integration/test_contacts.py
✅ Files skipped from review due to trivial changes (1)
- src/nc_mcp_server/tools/contacts.py
…rop unused nc_mcp fixture
…when both names cleared
6 tasks
This was referenced Apr 23, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add 6 Contacts tools via CardDAV protocol, completing another Phase 3 item:
list_addressbooksget_contactsget_contactcreate_contactupdate_contactdelete_contactUses
icalendarlibrary for vCard parsing/generation (already a dependency from Calendar).Strips PHOTO data from responses to keep context compact.
Parses multi-value fields (EMAIL, TEL, ADR) into typed arrays.
Test plan
Summary by CodeRabbit
New Features
Tests
Chores