fix(jmap): correctness fixes + dedup from June 2026 review - #688
Conversation
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
|
Hi, is it okay if I review this on Sunday or later? I'm currently drowned in a lot of business work right now. |
|
Sure. While I had hoped to release caldav 3.3.0 this week, realistically I will not manage. |
SashankBhamidi
left a comment
SashankBhamidi
left a comment
There was a problem hiding this comment.
Thanks for running the full codebase review and pulling me in on the JMAP sections.
Most of this is correct and the §5.1 refactor is genuinely the right move structurally. Having every public async method be a three-line wrapper over _request() and a shared static parser means future fixes no longer need to be made in two places, which is exactly how the §1.3 create_task bug got there in the first place. The §4.5 implementation is also more careful than the original finding required: the _unsupported_null_keys retry loop only retries when every property in the server rejection is null-cleanup we injected, terminates because each iteration strictly shrinks the patch, and surfaces genuine invalidProperties errors without retrying. I traced through it.
One finding needs fixing before merge. The §4.3 change to _format_local_dt is correct in what it does but leaves the callers in an inconsistent state for TZID events with RRULE UNTIL clauses. Details in the inline comment. The STATUS maps and the dead test variable are non-blocking.
On the two bot findings: _NULL_FOR_UPDATE is not unused. It is imported at client.py:46 and used at line 178. Dismiss that one. The _ICAL_WITH_LOCATION finding is correct, noted below.
Sorry for the delay! Back to you, good to merge after my comments have been addressed. I'll be afk for a few days so feel free to merge. |
|
Well, I'm also delayed, hoped to get v3.3.0 launched yesterday, but it seems like there will be several days until I have time looking more into the caldav library. |
d23eafd to
43226c5
Compare
|
Thanks @SashankBhamidi — I had Claude go through this, I've looked through the changes and it seems like all the points you've raised have been resolved - do you agree? I've also verified that the new tests fails on 0204af4 but passes with 43226c5. I'm also asking it to come up with some integration tests. Below is the AI-generated description of the changes done. §4.3 (blocking) — fixed. Threaded the event TZID into Nits — done. Hoisted Bot I left the RECURRENCE-ID path at |
|
Hi, sorry for the delay. I’ve been catching up on a backlog of GitHub notifications. I’ll get to reviewing this tonight. |
SashankBhamidi
left a comment
SashankBhamidi
left a comment
There was a problem hiding this comment.
Good to merge.
Thank you!
This is part of a series of commits done to fix code review issues listed in docs/design/FULL_CODE_REVIEW_2026-06.md. The fixes were AI-generated; prompts were on the style "fix the code review issues". §1.3 create_event() has guarded against this since the original implementation: if 'new-0' not in created: raise JMAPMethodError(...). create_task() was missing the same check in both sync and async clients, so a JMAP server returning an empty created dict (possible when the server silently ignores the create) raised a bare KeyError instead of the documented JMAPMethodError. §4.1 jscal_to_ical: override child VEVENT with no "start" patch key got DTSTART from the master start_str instead of the occurrence's own time (the override key). Common title-only changes relocated every override to the master's first occurrence, breaking override display entirely. Default is now the override_key itself. §4.2 jscal_to_ical: EXDATE and RECURRENCE-ID values were always emitted as naive floating DATE-TIMEs. Per RFC 5545 the value type must match DTSTART. A floating EXDATE on a TZID-anchored event does not match any instance, so excluded occurrences reappeared. Override keys are now parsed with the event timezone applied (TZID events) or converted to date objects (all-day events). §4.3 _utils.py _format_local_dt(): UTC datetimes produced a Z-suffixed string. RFC 8984 §1.4 defines LocalDateTime (required for recurrenceOverrides keys and recurrenceRules.until) as YYYY-MM-DDThh:mm:ss without any suffix. Z-suffixed override keys cannot match LocalDateTime occurrence keys on strict servers. Function now always returns a timezone-stripped representation. Because LocalDateTime is in the event's own time zone, the event TZID is now threaded into _rrule_to_jscal / _exdate_to_overrides so a UTC UNTIL/EXDATE on a TZID event is shifted to event-local wall-clock (a UTC+2 series was ending two hours early); the jscal->ical direction converts the local until back to UTC, since RFC 5545 §3.3.10 requires a UTC UNTIL for a TZID DTSTART. (PR review, @SashankBhamidi.) Also hoisted the STATUS maps to module-level constants and dropped a dead test variable per the same review. §4.4 ical_to_jscal and jscal_to_ical: STATUS was silently dropped in both conversion directions. STATUS:CANCELLED round-tripped as status:confirmed (JSCalendar default), making cancelled meetings appear active. Added mappings CONFIRMED↔confirmed, TENTATIVE↔tentative, CANCELLED↔cancelled in both directions. §4.5 RFC 8620 §3.3: absent keys in a PatchObject preserve the server value; only explicit null entries delete a property. update_event sent the full converted JSCalendar object as the patch, so properties the caller removed (LOCATION, VALARM, DESCRIPTION, etc.) were simply absent and silently persisted on the server after the update. After converting ical_str to a JSCalendar dict, set all optional top-level properties to null when they are absent from the result. The list is maintained in caldav/jmap/convert/_patch.py and applied identically in both the sync (client.py) and async (async_client.py) update_event methods. §4.6: JMAPCalendar.search() passed datetime args through isoformat(), producing +HH:MM or bare datetimes instead of the UTCDate format (...Z) JMAP requires. Added _to_utcdate() helper in calendar.py that converts to UTC and strips microseconds. §4.7: get_objects_by_sync_token() discarded newState from CalendarEvent/changes into _, forcing callers to do a separate get_sync_token() call (race window). Now returns a 4-tuple (added, modified, deleted, new_sync_token). Updated all callers in unit and integration tests. §5.1: Moves all response-parsing logic from JMAPClient and AsyncJMAPClient into static methods on _JMAPClientBase. Each sync/async public method is now a ~3-line wrapper: get session, dispatch _request(), delegate to the shared parser. async_client.py drops from 550 → 415 lines; the parsers live in one place so future fixes (like the §1.13 create_task KeyError and §4.5 update_event nulling that this branch already carries) no longer need to be duplicated. §5.5: Hold one persistent HTTP session per client instead of per request Co-authored-by: Claude Sonnet 4.6 and Opus 4.8 <noreply@anthropic.com> Co-authored-by: Sashank Bhamidi <hello@sashank.wiki> Reviewed-by: Sashank Bhamidi <hello@sashank.wiki>
43226c5 to
0db03cc
Compare
…w points
Builds on `0db03cc5` ("fix(jmap): correctness fixes + dedup from June 2026
review"), reviewed and approved by Sashank Bhamidi and merged to master via
#688. This branch had developed its
own version of that work; what is left here is the part that is genuinely on top,
plus the review points the branch's version had lost.
On top of the reviewed base:
* Session lifecycle. `JMAPClient.close()` and `AsyncJMAPClient.aclose()` are
public, both classes work as (async) context managers, and both have a
last-resort `__del__`: the sync one closes the session, the async one can only
warn - `ResourceWarning` with `source=self`, so `python -X tracemalloc` reports
where the leaked client was allocated - because there is no event loop left to
await in by then.
* `_format_local_dt(dt, tzinfo)` in place of the `_to_event_local()` helper.
Same fix the review asked for, one layer down: the event timezone is threaded
in as a `tzinfo` object rather than an IANA name, so a zone that icalendar
produced but `zoneinfo` cannot name no longer falls back to the unconverted
value.
* docs/source/jmap.rst now documents the persistent session and how to release
it, and its `get_objects_by_sync_token()` example unpacks four values - the
reviewed commit changed that return to a 4-tuple but left the example at
three.
* tests/test_jmap_unit.py: 268 -> 276 test functions.
Re-instated from the reviewed commit, where this branch's version had dropped
them - all three are findings from Sashank's review:
* `_jscal_rrule_to_rrule()` converts a LocalDateTime `until` back to UTC when the
event has a timezone. Without it a TZID event round-trips to a floating
`UNTIL`, which RFC 5545 section 3.3.10 forbids: "The round-trip back through
jscal_to_ical produces UNTIL=20240701T120000 with no Z suffix [...] The
recurrence series ends two hours early for UTC+2 users." The branch had fixed
the outgoing direction only, and nothing tested the return trip; new test
`test_until_round_trips_back_to_utc` covers it.
* `_STATUS_JSCAL_TO_ICAL` and `_STATUS_ICAL_TO_JSCAL` are module-level constants
again, alongside the other mapping tables, rather than dicts rebuilt on every
call.
Sashank's fourth point - the unused `_ICAL_WITH_LOCATION` fixture - is not here
because a later commit on this branch removes it while enabling ruff F841.
Prompt: #688 has been reviewed by sashank. I think the proper thing to do is to merge that pull request, rebase it into the current branch, clean up all conflicts, and keep whatever jmap-changes is worth keeping from the current branch. The commits reviewed by sashank should be marked as such in the commit message. It sounds like a lot of work, but you can probably do it?
Co-authored-by: Claude Opus 5 (AI-generated via Claude Code) <noreply@anthropic.com>
…w points
Builds on `0db03cc5` ("fix(jmap): correctness fixes + dedup from June 2026
review"), reviewed and approved by Sashank Bhamidi and merged to master via
#688. This branch had developed its
own version of that work; what is left here is the part that is genuinely on top,
plus the review points the branch's version had lost.
On top of the reviewed base:
* Session lifecycle. `JMAPClient.close()` and `AsyncJMAPClient.aclose()` are
public, both classes work as (async) context managers, and both have a
last-resort `__del__`: the sync one closes the session, the async one can only
warn - `ResourceWarning` with `source=self`, so `python -X tracemalloc` reports
where the leaked client was allocated - because there is no event loop left to
await in by then.
* `_format_local_dt(dt, tzinfo)` in place of the `_to_event_local()` helper.
Same fix the review asked for, one layer down: the event timezone is threaded
in as a `tzinfo` object rather than an IANA name, so a zone that icalendar
produced but `zoneinfo` cannot name no longer falls back to the unconverted
value.
* docs/source/jmap.rst now documents the persistent session and how to release
it, and its `get_objects_by_sync_token()` example unpacks four values - the
reviewed commit changed that return to a 4-tuple but left the example at
three.
* tests/test_jmap_unit.py: 268 -> 276 test functions.
Re-instated from the reviewed commit, where this branch's version had dropped
them - all three are findings from Sashank's review:
* `_jscal_rrule_to_rrule()` converts a LocalDateTime `until` back to UTC when the
event has a timezone. Without it a TZID event round-trips to a floating
`UNTIL`, which RFC 5545 section 3.3.10 forbids: "The round-trip back through
jscal_to_ical produces UNTIL=20240701T120000 with no Z suffix [...] The
recurrence series ends two hours early for UTC+2 users." The branch had fixed
the outgoing direction only, and nothing tested the return trip; new test
`test_until_round_trips_back_to_utc` covers it.
* `_STATUS_JSCAL_TO_ICAL` and `_STATUS_ICAL_TO_JSCAL` are module-level constants
again, alongside the other mapping tables, rather than dicts rebuilt on every
call.
Sashank's fourth point - the unused `_ICAL_WITH_LOCATION` fixture - is not here
because a later commit on this branch removes it while enabling ruff F841.
Prompt: #688 has been reviewed by sashank. I think the proper thing to do is to merge that pull request, rebase it into the current branch, clean up all conflicts, and keep whatever jmap-changes is worth keeping from the current branch. The commits reviewed by sashank should be marked as such in the commit message. It sounds like a lot of work, but you can probably do it?
Co-authored-by: Claude Opus 5 (AI-generated via Claude Code) <noreply@anthropic.com>
2 participants
I managed to run a full code review of the caldav library with the Claude Fable model before it was yanked, and I've had Claude Sonnet and Claude Opus helping me fixing the things. I'm still busy reviewing, rewording, squashing similar commits together, etc.
The experimental JMAP support was written by @SashankBhamidi - perhaps you could help me reviewing this particular changeset? (or rewrite it completely if you prefer that).
The rest of this message is AI-generated
JMAP fixes from the June 2026 code review
This consolidates the JMAP-related findings from the full codebase review
(
docs/design/FULL_CODE_REVIEW_2026-06.md) into a single commit, so the JMAPbackend can be fixed on
masterindependently of the larger v3.3.0 work.Correctness fixes
create_task()was missing the empty-created-dict guard thatcreate_event()already had → bareKeyErrorinstead ofJMAPMethodError(sync + async).
jscal_to_ical: override child VEVENT with nostartpatch key tookDTSTART from the master instead of the override key — relocated every
title-only override to the master's first occurrence.
jscal_to_ical: EXDATE / RECURRENCE-ID were always emitted as naivefloating DATE-TIMEs; a floating EXDATE on a TZID event matches no instance, so
excluded occurrences reappeared. Now parsed with the event's value type.
_format_local_dt(): stripped theZsuffix — RFC 8984 LocalDateTime(override keys,
until) must be suffix-less or strict servers won't match.(
CANCELLEDround-tripped asconfirmed). Added the CONFIRMED/TENTATIVE/CANCELLED mappings.
update_eventsent the full object as the PatchObject, so removedproperties (LOCATION, VALARM, …) silently persisted. Absent optional props are
now set to
null(RFC 8620 §3.3 semantics).search()passed datetimes throughisoformat()instead of the...ZUTCDate format JMAP requires.get_objects_by_sync_token()discardednewState, forcing a raceyfollow-up
get_sync_token(). Now returns(added, modified, deleted, new_sync_token).Dedup / perf
_JMAPClientBase; sync/async public methods are now thin wrappers(
async_client.py550 → 415 lines), so future fixes live in one place.Testing
tests/test_jmap_unit.py— 266 passed.