Skip to content

_merge_ical_todo_properties still rebuilds a VTODO from the partial update dict on any error #1252

Description

@cbcoutinho

_merge_ical_todo_properties (nextcloud_mcp_server/client/calendar.py:1817-1889) ends
in a catch-all handler that rebuilds the VTODO from the partial update dict:

except Exception as e:
    logger.error("Error merging iCal todo properties: %s", e)
    return self._create_ical_todo(todo_data, todo_uid)

todo_data on the update path carries only the fields the caller passed. Rebuilding from
it discards everything the caller did not happen to re-send — summary, due date, dtstart,
completed timestamp, categories, RRULE, alarms, and any custom property — while the tool
reports success. Any exception at all reaches this, including a transient parse failure
on an otherwise intact resource.

This is the same defect that was deliberately removed from the event merge as part of
the #544 fix. _merge_ical_properties now raises instead, and its docstring
(client/calendar.py:1499-1504) records why:

Raises on any merge failure rather than substituting a synthesised event. This
previously caught every exception and fell back to _create_ical_event(event_data, ...),
which rebuilds the event from the partial update dict — destroying summary, location,
attendees, alarms, RRULE and every custom property the caller did not happen to pass,
while reporting success.

The todo path never got the same treatment.

Fix

Drop the fallback and let the exception propagate, mirroring the event merge. A failed
update that surfaces as an error is strictly better than one that silently truncates the
todo. _create_ical_todo stays where it belongs, on the create path.

Worth checking in the same pass whether todo_uid is still needed as an argument once the
fallback is gone — it was removed from _merge_ical_properties for exactly that reason,
since the UID carries through from the stored iCal like any other preserved property.

Acceptance

  • _merge_ical_todo_properties raises rather than rebuilding from todo_data
  • A unit test asserts that a merge failure does not produce a truncated VTODO
  • Callers in update_todo surface the failure to the MCP client

Split out of #1251 to keep that one scoped to the dead parameters.

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions