fix(jmap): correctness fixes + dedup from June 2026 review

caldav/jmap/convert/_patch.py

@@ -0,0 +1,33 @@
+"""
+RFC 8620 PatchObject helpers for CalendarEvent/set update calls.
+
+When updating an event, absent keys preserve the server's current value.
+To delete an optional property the patch must set it to null explicitly.
+"""
+
+from __future__ import annotations
+
+# Optional JSCalendar top-level properties that must be explicitly nulled in
+# a CalendarEvent/set update when they are absent from the converted result.
+# This ensures properties removed client-side (e.g. LOCATION deleted from
+# the iCalendar) are actually removed on the server, not silently preserved.
+_NULL_FOR_UPDATE: frozenset[str] = frozenset(
+    {
+        "description",
+        "color",
+        "locations",
+        "keywords",
+        "priority",
+        "privacy",
+        "freeBusyStatus",
+        "status",
+        "sequence",
+        "showWithoutTime",
+        "timeZone",
+        "recurrenceRules",
+        "excludedRecurrenceRules",
+        "recurrenceOverrides",
+        "participants",
+        "alerts",
+    }
+)

caldav/jmap/convert/_utils.py

@@ -111,22 +111,21 @@ def _duration_to_timedelta(duration_str: str) -> timedelta:
 
 
 def _format_local_dt(dt: datetime | date) -> str:
-    """Format a datetime or date as a JSCalendar LocalDateTime or UTCDateTime string.
+    """Format a datetime or date as a JSCalendar LocalDateTime string.
 
-    JSCalendar uses:
-      - LocalDateTime: "2024-03-15T09:00:00"    (no TZ suffix)
-      - UTCDateTime:   "2024-03-15T09:00:00Z"   (uppercase Z)
+    RFC 8984 requires LocalDateTime (no Z suffix) for override keys and RRULE
+    ``until`` values.  Timezone information is stripped — callers must convert
+    UTC datetimes to the event's local timezone before calling if the event uses
+    TZID; for floating or all-day events the naive value is already correct.
 
     For date objects (all-day), uses T00:00:00 suffix.
 
     Args:
         dt: A datetime (with or without tzinfo) or a date.
 
     Returns:
-        Formatted string suitable for use as a JSCalendar override key or datetime value.
+        Formatted string suitable for use as a JSCalendar override key or RRULE until.
     """
     if isinstance(dt, datetime):
-        if dt.tzinfo is not None and dt.utcoffset() == timedelta(0):
-            return dt.strftime("%Y-%m-%dT%H:%M:%SZ")
         return dt.strftime("%Y-%m-%dT%H:%M:%S")
     return f"{dt.isoformat()}T00:00:00"

caldav/jmap/convert/ical_to_jscal.py

@@ -386,6 +386,17 @@ def ical_to_jscal(ical_str: str, calendar_id: str | None = None) -> dict:
     if location:
         jscal["locations"] = _location_str_to_jscal(str(location))
 
+    status = master.get("STATUS")
+    if status:
+        _STATUS_ICAL_TO_JSCAL = {
+            "CONFIRMED": "confirmed",
+            "TENTATIVE": "tentative",
+            "CANCELLED": "cancelled",
+        }
+        jscal_status = _STATUS_ICAL_TO_JSCAL.get(str(status).upper())
+        if jscal_status:
+            jscal["status"] = jscal_status
+
     participants: dict = {}
     organizer = master.get("ORGANIZER")
     if organizer is not None:

caldav/jmap/convert/jscal_to_ical.py

@@ -353,6 +353,17 @@ def jscal_to_ical(jscal: dict) -> str:
         if loc_name:
             event.add("location", loc_name)
 
+    status = jscal.get("status")
+    if status:
+        _STATUS_JSCAL_TO_ICAL = {
+            "confirmed": "CONFIRMED",
+            "tentative": "TENTATIVE",
+            "cancelled": "CANCELLED",
+        }
+        ical_status = _STATUS_JSCAL_TO_ICAL.get(status)
+        if ical_status:
+            event.add("status", ical_status)
+
     for rule in jscal.get("recurrenceRules") or []:
         ical_rule = _jscal_rrule_to_rrule(rule)
         if ical_rule:
@@ -371,6 +382,15 @@ def jscal_to_ical(jscal: dict) -> str:
             rid_dt: datetime | date = datetime.strptime(override_key, "%Y-%m-%dT%H:%M:%SZ").replace(
                 tzinfo=timezone.utc
             )
+        elif show_without_time:
+            rid_dt = date.fromisoformat(override_key[:10])
+        elif time_zone:
+            try:
+                rid_dt = datetime.strptime(override_key[:19], "%Y-%m-%dT%H:%M:%S").replace(
+                    tzinfo=ZoneInfo(time_zone)
+                )
+            except ZoneInfoNotFoundError:
+                rid_dt = datetime.strptime(override_key[:19], "%Y-%m-%dT%H:%M:%S")
         else:
             rid_dt = datetime.strptime(override_key[:19], "%Y-%m-%dT%H:%M:%S")
 
@@ -381,7 +401,8 @@ def jscal_to_ical(jscal: dict) -> str:
             child.add("uid", uid)
             child.add("dtstamp", datetime.now(tz=timezone.utc))
             child.add("recurrence-id", rid_dt)
-            child_start = patch.get("start", start_str)
+            # Default child start to the occurrence time (override key), not the master start.
+            child_start = patch.get("start", override_key)
             child_tz = patch.get("timeZone", time_zone)
             child_swt = patch.get("showWithoutTime", show_without_time)
             if child_start:

caldav/jmap/objects/calendar.py

@@ -8,7 +8,7 @@
 from __future__ import annotations
 
 from dataclasses import dataclass, field
-from datetime import datetime
+from datetime import datetime, timezone
 from typing import TYPE_CHECKING
 
 from caldav.jmap.objects.calendar_object import JMAPCalendarObject
@@ -18,6 +18,17 @@
     from caldav.jmap.client import JMAPClient
 
 
+def _to_utcdate(dt: datetime) -> str:
+    """Convert a datetime to JMAP UTCDate format (YYYY-MM-DDTHH:MM:SSZ).
+
+    Naive datetimes are assumed to be UTC.  Aware datetimes are converted to
+    UTC before formatting.  Microseconds are dropped as JMAP does not allow them.
+    """
+    if dt.tzinfo is None:
+        dt = dt.replace(tzinfo=timezone.utc)
+    return dt.astimezone(timezone.utc).strftime("%Y-%m-%dT%H:%M:%SZ")
+
+
 @dataclass
 class JMAPCalendar:
     """A JMAP Calendar object.
@@ -111,9 +122,9 @@ def search(self, **searchargs):
         start = searchargs.get("start")
         end = searchargs.get("end")
         if isinstance(start, datetime):
-            start = start.isoformat()
+            start = _to_utcdate(start)
         if isinstance(end, datetime):
-            end = end.isoformat()
+            end = _to_utcdate(end)
         return self._client._search(
             calendar_id=self.id,
             start=start,
@@ -126,9 +137,9 @@ async def _async_search(self, **searchargs) -> list[JMAPCalendarObject]:
         start = searchargs.get("start")
         end = searchargs.get("end")
         if isinstance(start, datetime):
-            start = start.isoformat()
+            start = _to_utcdate(start)
         if isinstance(end, datetime):
-            end = end.isoformat()
+            end = _to_utcdate(end)
         return await self._client._search(
             calendar_id=self.id,
             start=start,

docs/design/FULL_CODE_REVIEW_2026-06.md

@@ -138,7 +138,7 @@ Truncated/garbage iCalendar without DTSTAMP and without an `END:` line makes
 the DTSTAMP fixup logic it guards) is silently skipped. Should be
 `error.assert_` or a proper parse error.
 
-### 1.13 `jmap/client.py:576` / `jmap/async_client.py:461` — `create_task` missing the guard `create_event` has `[code]`
+### 1.13 `jmap/client.py:576` / `jmap/async_client.py:461` — `create_task` missing the guard `create_event` has `[code]` ✅ FIXED
 `create_event` handles an empty `created` dict with a descriptive
 `JMAPMethodError` (`client.py:294–298`); `create_task` does
 `created["new-0"]["id"]` unguarded → bare KeyError, bypassing the JMAP error
@@ -308,7 +308,7 @@ explicitly).
 bodies (calendar PII, custom auth headers) to files that accumulate
 indefinitely. Files are 0600, and the niquests-applied Authorization header
 is added after the dump point, so exposure is limited — but a cleanup policy
-or a documented warning would be appropriate.
+or a documented warning would be appropriate.  **Human notes:** This is in /tmp, so we should expect some kind of cleanup on the OS level.  There does exist some security notes in the CHANGELOG for the revision adding the feature, but the CHANGELOG has been pruned, so it's needed to consult git history to find it - it should definitively be lifted up to a more visible place.
 
 **Ruled out** (checked, found safe): SSRF via server-returned hrefs
 (`_normalize_href` reduces absolute URLs to path-only); credential leak on
@@ -319,32 +319,32 @@ cross-host redirects (auth applied via auth callable, stripped by
 
 ## 4. JMAP backend
 
-### 4.1 `jmap/convert/jscal_to_ical.py:384` — override child VEVENT gets the master's DTSTART `[repro]`
+### 4.1 `jmap/convert/jscal_to_ical.py:384` — override child VEVENT gets the master's DTSTART `[repro]` ✅ FIXED
 `child_start = patch.get("start", start_str)` defaults to the master start.
 An override that doesn't move the occurrence (e.g. title-only change — the
 common case) renders a child VEVENT with RECURRENCE-ID at the occurrence but
 DTSTART at the *master's* start, relocating the occurrence. Default must be
 the override key (`rid_dt`).
 
-### 4.2 `jmap/convert/jscal_to_ical.py:375` — EXDATE/RECURRENCE-ID value-type mismatch `[repro]`
+### 4.2 `jmap/convert/jscal_to_ical.py:375` — EXDATE/RECURRENCE-ID value-type mismatch `[repro]` ✅ FIXED
 Override keys are rendered as naive floating DATE-TIMEs regardless of the
 event's `timeZone`/`showWithoutTime`: a TZID-anchored event gets
 `EXDATE:20260620T100000` (floating — per RFC 5545 it does not match the
 instance, so the **excluded occurrence reappears**), and an all-day event
 gets a DATETIME EXDATE against a `VALUE=DATE` DTSTART.
 
-### 4.3 `jmap/convert/ical_to_jscal.py:100` (via `_utils.py:129`) — `Z`-suffix in LocalDateTime slots `[repro]`
+### 4.3 `jmap/convert/ical_to_jscal.py:100` (via `_utils.py:129`) — `Z`-suffix in LocalDateTime slots `[repro]` ✅ FIXED
 UTC inputs produce `...Z` strings for RRULE `until` and recurrenceOverrides
 keys; RFC 8984 requires LocalDateTime there. Strict servers reject with
 `invalidArguments`; lenient ones mis-set the boundary, and a `Z`-suffixed
 override key can never match a LocalDateTime occurrence key.
 
-### 4.4 `jmap/convert/*` — STATUS dropped in both directions `[code]`
+### 4.4 `jmap/convert/*` — STATUS dropped in both directions `[code]` ✅ FIXED
 Neither converter maps `STATUS` ↔ `status` (only
 participationStatus/freeBusyStatus exist). `STATUS:CANCELLED` round-trips to
 the JSCalendar default `confirmed`; cancelled meetings come back as active.
 
-### 4.5 `jmap/client.py:346` / `async_client.py:233` — `update_event` patch never clears removed properties `[code]`
+### 4.5 `jmap/client.py:346` / `async_client.py:233` — `update_event` patch never clears removed properties `[code]` ✅ FIXED
 The full converted object is sent as the RFC 8620 PatchObject; the converter
 only includes keys conditionally, so a property deleted client-side (e.g.
 LOCATION, VALARM) is simply *absent* from the patch and **persists on the
@@ -373,7 +373,7 @@ producing drift bugs.
    modulo `await`. The build-side is already shared via `_JMAPClientBase` /
    `jmap/_methods`; moving the response-parsing glue into shared pure
    methods would shrink each sync/async method to ~3 lines. (The §1.13 and
-   §4.5 bugs are duplicated exactly because of this.)
+   §4.5 bugs are duplicated exactly because of this.) ✅ FIXED (commit ed3c47b0)
 2. **`calendarobjectresource.py:1166–1205` — `_post_put` block pasted twice
    in sequence**; the second `elif r.status not in (204, 201)` is
    unreachable. Also factor the Etag/Schedule-Tag header→props snippet
@@ -390,7 +390,7 @@ producing drift bugs.
 5. **JMAP clients open a fresh HTTP connection per request** (async:
    `async with AsyncSession()` per `_request`; sync: module-level
    `requests.post`). `__exit__`/`__aexit__` already exist but do nothing —
-   hold one session in `_JMAPClientBase` and close it there.
+   hold one session in `_JMAPClientBase` and close it there. ✅ FIXED
 6. **`Todo._async_complete_recurring_thisandfuture` copies ~60 lines of its
    sync twin** (the file says "TERRIBLY much code duplication here"), and
    the async safe-variant has drifted: it PUTs the completed copy twice.
@@ -403,7 +403,7 @@ producing drift bugs.
    purelymail 404) must be maintained twice; the TODO at line 577 already
    acknowledges this.
 8. **`search.py` sync/async driver loops duplicated** (~80 lines including
-   the Phase-1/Phase-2 exception-rethrow protocol and
+   the hase-1/Phase-2 exception-rethrow protocol and
    `_search_with_comptypes`). A small executor object with sync/async
    implementations would leave one driver.
 
@@ -424,6 +424,8 @@ producing drift bugs.
    regression-testing each fixup against the exact server output it was
    written for.
 
+   **Human comment:** we've been discussing a bit moving this logic into the icalendar library - but it's hard to make good solutions, so we'll need to keep the stop-gap implementation as for now.
+
 ---
 
 ## 7. Recommended priorities

tests/test_jmap_integration.py

@@ -220,7 +220,7 @@ def test_event_sync(self, client, calendar_id):
         token_before = client.get_sync_token()
         event_id = client.create_event(calendar_id, _minimal_ical("Sync Test Event"))
         try:
-            added, _modified, _deleted = client.get_objects_by_sync_token(token_before)
+            added, _modified, _deleted, _new_token = client.get_objects_by_sync_token(token_before)
             assert any("Sync Test Event" in jscal_to_ical(a.get_data()) for a in added)
         finally:
             client.delete_event(event_id)
@@ -280,7 +280,9 @@ async def test_event_sync(self, async_client, async_calendar_id):
             async_calendar_id, _minimal_ical("Async Sync Test Event")
         )
         try:
-            added, _modified, _deleted = await async_client.get_objects_by_sync_token(token_before)
+            added, _modified, _deleted, _new_token = await async_client.get_objects_by_sync_token(
+                token_before
+            )
             assert any("Async Sync Test Event" in jscal_to_ical(a.get_data()) for a in added)
         finally:
             await async_client.delete_event(event_id)