diff --git a/src/fabledassistant/routes/events.py b/src/fabledassistant/routes/events.py index 63ca1b0..600ba63 100644 --- a/src/fabledassistant/routes/events.py +++ b/src/fabledassistant/routes/events.py @@ -54,19 +54,22 @@ async def create_event(): end_dt = _parse_dt(data["end_dt"]) if data.get("end_dt") else None except ValueError: return jsonify({"error": "Invalid datetime format"}), 400 - event = await events_svc.create_event( - user_id=_get_current_user_id(), - title=data["title"], - start_dt=start_dt, - end_dt=end_dt, - all_day=data.get("all_day", False), - description=data.get("description", ""), - location=data.get("location", ""), - color=data.get("color", ""), - recurrence=data.get("recurrence"), - project_id=data.get("project_id"), - reminder_minutes=data.get("reminder_minutes"), - ) + try: + event = await events_svc.create_event( + user_id=_get_current_user_id(), + title=data["title"], + start_dt=start_dt, + end_dt=end_dt, + all_day=data.get("all_day", False), + description=data.get("description", ""), + location=data.get("location", ""), + color=data.get("color", ""), + recurrence=data.get("recurrence"), + project_id=data.get("project_id"), + reminder_minutes=data.get("reminder_minutes"), + ) + except ValueError as exc: + return jsonify({"error": str(exc)}), 400 return jsonify(event.to_dict()), 201 @@ -106,11 +109,14 @@ async def update_event(event_id: int): fields[dt_field] = _parse_dt(data[dt_field]) except ValueError: return jsonify({"error": f"Invalid datetime for {dt_field}"}), 400 - event = await events_svc.update_event( - user_id=_get_current_user_id(), - event_id=event_id, - **fields, - ) + try: + event = await events_svc.update_event( + user_id=_get_current_user_id(), + event_id=event_id, + **fields, + ) + except ValueError as exc: + return jsonify({"error": str(exc)}), 400 if event is None: return jsonify({"error": "Event not found"}), 404 return jsonify(event.to_dict()) diff --git a/src/fabledassistant/services/events.py b/src/fabledassistant/services/events.py index 381e655..c5c9a43 100644 --- a/src/fabledassistant/services/events.py +++ b/src/fabledassistant/services/events.py @@ -32,7 +32,17 @@ async def create_event( attendees: list[str] | None = None, calendar_name: str | None = None, ) -> Event: - """Create an event in the DB, then fire a CalDAV push task.""" + """Create an event in the DB, then fire a CalDAV push task. + + Raises ValueError if `end_dt` is set but not strictly after `start_dt`. + Catching this at write time prevents the "end before start" data state + that breaks downstream filters (Fable #160). + """ + if end_dt is not None and end_dt <= start_dt: + raise ValueError( + f"end_dt ({end_dt.isoformat()}) must be after start_dt " + f"({start_dt.isoformat()}); pass end_dt=None for point events." + ) uid = str(uuid.uuid4()) async with async_session() as session: event = Event( @@ -87,12 +97,16 @@ async def list_events( async with async_session() as session: # Match strategy: # - Recurring events: fetch all, expand via rrule below. - # - Non-recurring with an end_dt: standard overlap — starts before - # date_to and ends after date_from. - # - Non-recurring with no end_dt: treat as a point event at - # start_dt, include only if start_dt falls within the window. - # (Previously this branch matched any event with a null end_dt, - # returning all past events as "happening today".) + # - Non-recurring with a VALID end_dt (end > start): standard + # overlap — starts before date_to and ends after date_from. + # - Non-recurring with no end_dt OR an invalid end_dt (end <= + # start; almost always a tool-call mishap): treat as a point + # event at start_dt and include only if start_dt is in the + # window. The fallback for invalid end_dt is critical — without + # it, an end_dt accidentally set in the past makes the event + # vanish from every list_events call regardless of where its + # start_dt actually is. Discovered 2026-04-29; see Fable #160 + # for the structural fix that replaces end_dt with duration. result = await session.execute( select(Event).where( Event.user_id == user_id, @@ -102,9 +116,16 @@ async def list_events( Event.recurrence.is_(None), Event.start_dt <= date_to, or_( - Event.end_dt >= date_from, and_( - Event.end_dt.is_(None), + Event.end_dt.isnot(None), + Event.end_dt > Event.start_dt, + Event.end_dt >= date_from, + ), + and_( + or_( + Event.end_dt.is_(None), + Event.end_dt <= Event.start_dt, + ), Event.start_dt >= date_from, ), ), @@ -120,8 +141,14 @@ async def list_events( items.append(event.to_dict()) continue - # Expand recurring event occurrences within [date_from, date_to] - duration = (event.end_dt - event.start_dt) if event.end_dt else None + # Expand recurring event occurrences within [date_from, date_to]. + # Treat end_dt <= start_dt as no-end (invalid data); falling back + # to point events instead of computing a negative duration. + duration = ( + (event.end_dt - event.start_dt) + if event.end_dt and event.end_dt > event.start_dt + else None + ) try: rule = rrulestr(event.recurrence, dtstart=event.start_dt, ignoretz=False) occurrences = rule.between(date_from, date_to, inc=True) @@ -173,7 +200,13 @@ async def search_events( async def update_event(user_id: int, event_id: int, **fields) -> Event | None: - """Partial update. Returns updated event or None if not found.""" + """Partial update. Returns updated event or None if not found. + + Raises ValueError if the resulting state would have `end_dt <= start_dt`. + Validation runs against the *post-update* values so updates that fix + one side of the constraint without explicitly clearing the other still + fail loudly instead of persisting invalid data. + """ async with async_session() as session: result = await session.execute( select(Event).where(Event.id == event_id, Event.user_id == user_id) @@ -186,6 +219,17 @@ async def update_event(user_id: int, event_id: int, **fields) -> Event | None: "location", "color", "recurrence", "project_id", "reminder_minutes"} # Nullable fields that callers can explicitly set to None to clear nullable = {"end_dt", "recurrence", "project_id", "reminder_minutes"} + + # Pre-validate the *resulting* start/end against the post-update + # state, not against either field's pre-update value alone. + new_start = fields["start_dt"] if "start_dt" in fields and fields["start_dt"] is not None else event.start_dt + new_end = fields["end_dt"] if "end_dt" in fields else event.end_dt + if new_end is not None and new_end <= new_start: + raise ValueError( + f"end_dt ({new_end.isoformat()}) must be after start_dt " + f"({new_start.isoformat()}); pass end_dt=None for point events." + ) + for key, value in fields.items(): if key in allowed and (value is not None or key in nullable): setattr(event, key, value) diff --git a/src/fabledassistant/services/tools/calendar.py b/src/fabledassistant/services/tools/calendar.py index 2f15de9..8f37268 100644 --- a/src/fabledassistant/services/tools/calendar.py +++ b/src/fabledassistant/services/tools/calendar.py @@ -213,22 +213,25 @@ async def create_event_tool(*, user_id, arguments, **_ctx): proj = await resolve_project(user_id, project_name) if proj: project_id = proj.id - event = await events_create_event( - user_id=user_id, - title=arguments.get("title", "Untitled Event"), - start_dt=start_dt, - end_dt=end_dt, - all_day=all_day, - description=arguments.get("description") or "", - location=arguments.get("location") or "", - color=arguments.get("color") or "", - recurrence=arguments.get("recurrence"), - project_id=project_id, - duration=arguments.get("duration"), - reminder_minutes=arguments.get("reminder_minutes"), - attendees=arguments.get("attendees"), - calendar_name=arguments.get("calendar_name"), - ) + try: + event = await events_create_event( + user_id=user_id, + title=arguments.get("title", "Untitled Event"), + start_dt=start_dt, + end_dt=end_dt, + all_day=all_day, + description=arguments.get("description") or "", + location=arguments.get("location") or "", + color=arguments.get("color") or "", + recurrence=arguments.get("recurrence"), + project_id=project_id, + duration=arguments.get("duration"), + reminder_minutes=arguments.get("reminder_minutes"), + attendees=arguments.get("attendees"), + calendar_name=arguments.get("calendar_name"), + ) + except ValueError as exc: + return {"success": False, "error": str(exc)} return {"success": True, "type": "event", "data": event.to_dict()} @@ -360,7 +363,10 @@ async def update_event_tool(*, user_id, arguments, **_ctx): return {"success": False, "error": f"Invalid end: {exc}"} if end_dt is not None: fields["end_dt"] = end_dt - updated = await events_update_event(user_id=user_id, event_id=event_to_update.id, **fields) + try: + updated = await events_update_event(user_id=user_id, event_id=event_to_update.id, **fields) + except ValueError as exc: + return {"success": False, "error": str(exc)} if updated is None: return {"success": False, "error": "Event not found or update failed."} return {"success": True, "type": "event_updated", "data": updated.to_dict()} diff --git a/tests/test_events_service.py b/tests/test_events_service.py index bf34538..2b6ea44 100644 --- a/tests/test_events_service.py +++ b/tests/test_events_service.py @@ -124,6 +124,92 @@ async def test_update_event_fires_caldav_push(): assert mock_task.called +@pytest.mark.asyncio +async def test_create_event_rejects_end_before_start(): + """Write-side validation: end_dt <= start_dt must raise rather than + persisting invalid data. Discovered 2026-04-29: a tool-call mishap + left an event with end_dt 32 days BEFORE start_dt; the bad data then + made the event invisible to the upcoming-list filter. Catching at + the boundary prevents the same shape from recurring.""" + from fabledassistant.services.events import create_event + start = datetime(2026, 5, 1, 12, 0, tzinfo=timezone.utc) + end_before = datetime(2026, 3, 30, 12, 0, tzinfo=timezone.utc) + with pytest.raises(ValueError, match="must be after start_dt"): + await create_event( + user_id=1, title="Bad", + start_dt=start, end_dt=end_before, + ) + + +@pytest.mark.asyncio +async def test_create_event_rejects_end_equal_to_start(): + """Equal start/end is also invalid (zero-duration); pass end=None + for point events instead.""" + from fabledassistant.services.events import create_event + same = datetime(2026, 5, 1, 12, 0, tzinfo=timezone.utc) + with pytest.raises(ValueError, match="must be after start_dt"): + await create_event( + user_id=1, title="Zero", + start_dt=same, end_dt=same, + ) + + +@pytest.mark.asyncio +async def test_update_event_rejects_post_state_with_end_before_start(): + """update_event must validate against the *post-update* state, not + just the field passed in. A user changing only start_dt to a value + after the existing end_dt would otherwise sneak past validation.""" + mock_event = _make_mock_event() # start = 2026-03-25 10:00, end = 11:00 + mock_session = _make_mock_session() + mock_result = MagicMock() + mock_result.scalar_one_or_none.return_value = mock_event + mock_session.execute = AsyncMock(return_value=mock_result) + + with patch("fabledassistant.services.events.async_session") as mock_cls: + mock_cls.return_value = mock_session + from fabledassistant.services.events import update_event + # Push start_dt past the existing end_dt (without touching end_dt) + with pytest.raises(ValueError, match="must be after start_dt"): + await update_event( + user_id=1, event_id=1, + start_dt=datetime(2026, 3, 25, 12, 0, tzinfo=timezone.utc), + ) + + +@pytest.mark.asyncio +async def test_list_events_returns_event_with_invalid_end_dt(): + """Filter robustness: an event whose end_dt landed before start_dt + (corrupt data state from earlier bugs) must still surface in the + upcoming list when its start_dt is in range. Without this, the + event becomes invisible everywhere except via direct id lookup.""" + mock_event = _make_mock_event() + # Corrupt state: end is 32 days BEFORE start + mock_event.start_dt = datetime(2026, 5, 1, 12, 0, tzinfo=timezone.utc) + mock_event.end_dt = datetime(2026, 3, 30, 12, 0, tzinfo=timezone.utc) + mock_event.to_dict.return_value = { + "id": 1, "title": "Corrupt event", + "start_dt": mock_event.start_dt.isoformat(), + "end_dt": mock_event.end_dt.isoformat(), + } + mock_session = _make_mock_session() + mock_result = MagicMock() + mock_result.scalars.return_value.all.return_value = [mock_event] + mock_session.execute = AsyncMock(return_value=mock_result) + + with patch("fabledassistant.services.events.async_session") as mock_cls: + mock_cls.return_value = mock_session + from fabledassistant.services.events import list_events + # Window covers start_dt (May 1) but not end_dt (March 30). + results = await list_events( + user_id=1, + date_from=datetime(2026, 4, 29, tzinfo=timezone.utc), + date_to=datetime(2026, 5, 27, tzinfo=timezone.utc), + ) + # Pre-fix: filter would exclude this event. Post-fix: present. + assert len(results) == 1 + assert results[0]["id"] == 1 + + @pytest.mark.asyncio async def test_tools_calendar_always_available(): """Calendar tools must appear in get_tools_for_user even without CalDAV."""