From 84640a0dc4a96371cce20ac8968bf47623d91fb1 Mon Sep 17 00:00:00 2001 From: Bryan Van Deusen Date: Wed, 29 Apr 2026 14:29:13 -0400 Subject: [PATCH] fix(events-tools): refuse ambiguous query, require event_id to disambiguate (#161) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Root cause of the 2026-04-29 dentist-appointment incident: the model called update_event(query="Appointment") when two events had "Appointment" in their titles. find_events_by_query returned both, upcoming-first ordered by start_dt — matches[0] was id=2 (a stale pre-existing event with garbage end_dt), not id=15 (the one the user just created via the journal flow). update_event_tool silently took matches[0] and mutated the wrong event. Fix: a new resolver helper `_resolve_event_for_action` funnels both update_event_tool and delete_event_tool through one disambiguation path. Lookup precedence: - `event_id` → exact get_event lookup, no query at all - `query` matching exactly one event → proceed - `query` matching zero → return success=False, "no event found" - `query` matching 2+ events → return success=False with a `candidates` array of {id, title, start_dt, location} so the model can pick one and call again with `event_id` The candidates list is capped at 8 to keep the model's context tight. The error message names the count and the next-step ("pass event_id or refine the query") so the model can self-correct in one turn. For delete_event, the disambiguation is even more important — the silent-matches[0] path would have deleted the wrong event outright rather than just mutating it. The tool description leans into that: "Deleting the wrong event is a costly user error; never guess." Tool surface change: `query` and `event_id` are now both optional; the tool errors clearly when neither is supplied. The model already knows id values from prior tool results (returned in `data.id`), which is the natural feeder for the disambiguation flow. 5 new tests in test_calendar_tool_tz.py cover: - ambiguous query → success=False with candidate list, no mutation - event_id supplied → bypasses query lookup entirely - non-existent event_id → clear "no event found" error - neither identifier → "query or event_id required" error - same disambiguation enforced for delete_event_tool 46 calendar/events tests pass; ruff clean. Closes Fable #161. Co-Authored-By: Claude Opus 4.7 --- .../services/tools/calendar.py | 133 +++++++++++-- tests/test_calendar_tool_tz.py | 182 ++++++++++++++++++ 2 files changed, 299 insertions(+), 16 deletions(-) diff --git a/src/fabledassistant/services/tools/calendar.py b/src/fabledassistant/services/tools/calendar.py index 2f15de9..6149854 100644 --- a/src/fabledassistant/services/tools/calendar.py +++ b/src/fabledassistant/services/tools/calendar.py @@ -108,6 +108,88 @@ async def _resolve_event_end( return None +def _candidate_summary(event) -> dict: + """Compact event summary used in ambiguous-match responses. + + Keeps the candidate list small so the model can disambiguate from + the same turn without bloating context. Includes id (for the + follow-up call), title, start_dt, and location when present. + """ + return { + "id": event.id, + "title": event.title, + "start_dt": event.start_dt.isoformat() if event.start_dt else None, + "location": event.location or None, + } + + +async def _resolve_event_for_action( + *, user_id: int, arguments: dict, action: str, +): + """Pick the single event the model intends to update or delete. + + Resolution rules: + - ``event_id`` in arguments → exact lookup (skip query). Used by + the model to disambiguate after a multi-match refusal. + - else ``query`` → ``find_events_by_query``: + - 0 results → return error tuple ("not_found", ...) + - 1 result → return that event + - 2+ results → return ("ambiguous", error, candidates) so the + caller can refuse the call and show candidates to the model. + + Returns either an Event (success) or a 2- or 3-tuple of + ``(error_kind, error_dict)`` for the caller to translate into a + tool-call response. + """ + from fabledassistant.services.events import get_event + + event_id = arguments.get("event_id") + if event_id is not None: + try: + event_id_int = int(event_id) + except (TypeError, ValueError): + return ("invalid_id", { + "success": False, + "error": f"event_id must be an integer; got {event_id!r}.", + }) + ev = await get_event(user_id=user_id, event_id=event_id_int) + if ev is None: + return ("not_found", { + "success": False, + "error": f"No event found with id={event_id_int}.", + }) + return ev + + query = arguments.get("query", "") + if not query: + return ("invalid_query", { + "success": False, + "error": "Either query or event_id is required.", + }) + matches = await find_events_by_query(user_id=user_id, query=query) + if not matches: + return ("not_found", { + "success": False, + "error": f"No event found matching {query!r}.", + }) + if len(matches) == 1: + return matches[0] + # Multi-match: refuse and surface candidates so the model can + # disambiguate via event_id on the next call. Prevents the silent- + # picks-matches[0] failure mode that mutated the wrong event in the + # 2026-04-29 dentist-appointment incident (Fable #161). + return ("ambiguous", { + "success": False, + "error": ( + f"Found {len(matches)} events matching {query!r}. " + f"Pick one by passing `event_id` instead of `query`, " + f"or refine the search term to match a single event." + ), + "action": action, + "candidates": [_candidate_summary(m) for m in matches[:8]], + }) + + def _validate_weekday(start_dt_utc: datetime, user_tz, expected: str | None) -> str | None: """Verify the resolved local date falls on the expected day of the week. @@ -305,10 +387,17 @@ async def search_events_tool(*, user_id, arguments, **_ctx): "When the user names a weekday ('move to Friday'), state the " "resolved calendar date in your reply BEFORE calling this tool, " "and pass `expected_weekday` so the server can verify the date " - "falls on the day you intended." + "falls on the day you intended.\n\n" + "Identify the event with EITHER `query` (a title substring) OR " + "`event_id` (when you already have an exact id from a prior tool " + "result). If `query` matches multiple events, the tool returns " + "an ambiguity error with a candidate list — pick one by passing " + "its `event_id` on the next call, or refine the query so it " + "matches a single event." ), parameters={ - "query": {"type": "string", "description": "Search term to find the event to update (matches against title)"}, + "query": {"type": "string", "description": "Search term to find the event to update (matches against title). Required unless event_id is set."}, + "event_id": {"type": "integer", "description": "Exact event id, used to disambiguate when a prior call returned multiple candidates. Takes precedence over query."}, "title": {"type": "string", "description": "New title for the event"}, "start_date": {"type": "string", "description": "New start calendar date as YYYY-MM-DD in the user's local time. No timezone suffix."}, "start_time": {"type": "string", "description": "New start wall-clock time as HH:MM. No timezone suffix."}, @@ -325,14 +414,15 @@ async def search_events_tool(*, user_id, arguments, **_ctx): "start": {"type": "string", "description": "[Deprecated] Combined start datetime — prefer start_date + start_time."}, "end": {"type": "string", "description": "[Deprecated] Combined end datetime — prefer end_date + end_time."}, }, - required=["query"], + required=[], ) async def update_event_tool(*, user_id, arguments, **_ctx): - query = arguments.get("query", "") - matches = await find_events_by_query(user_id=user_id, query=query) - if not matches: - return {"success": False, "error": f"No event found matching '{query}'."} - event_to_update = matches[0] + resolved = await _resolve_event_for_action( + user_id=user_id, arguments=arguments, action="update", + ) + if isinstance(resolved, tuple): + return resolved[1] # error dict from the resolver + event_to_update = resolved fields: dict = {} for str_field in ("title", "description", "location", "color", "recurrence"): if arguments.get(str_field) is not None: @@ -368,18 +458,29 @@ async def update_event_tool(*, user_id, arguments, **_ctx): @tool( name="delete_event", - description="Delete a calendar event. Use this when the user asks to cancel, remove, or delete an event.", + description=( + "Delete a calendar event. Use this when the user asks to cancel, " + "remove, or delete an event. Identify the event with EITHER " + "`query` (a title substring) OR `event_id` (when you have an " + "exact id). If `query` matches multiple events, the tool returns " + "an ambiguity error with a candidate list — pick one by passing " + "its `event_id` on the next call, or refine the query so it " + "matches a single event. Deleting the wrong event is a costly " + "user error; never guess between candidates." + ), parameters={ - "query": {"type": "string", "description": "Search term to find the event to delete (matches against title)"}, + "query": {"type": "string", "description": "Search term to find the event to delete (matches against title). Required unless event_id is set."}, + "event_id": {"type": "integer", "description": "Exact event id, used to disambiguate when a prior call returned multiple candidates. Takes precedence over query."}, }, - required=["query"], + required=[], ) async def delete_event_tool(*, user_id, arguments, **_ctx): - query = arguments.get("query", "") - matches = await find_events_by_query(user_id=user_id, query=query) - if not matches: - return {"success": False, "error": f"No event found matching '{query}'."} - event_to_delete = matches[0] + resolved = await _resolve_event_for_action( + user_id=user_id, arguments=arguments, action="delete", + ) + if isinstance(resolved, tuple): + return resolved[1] + event_to_delete = resolved await events_delete_event(user_id=user_id, event_id=event_to_delete.id) return {"success": True, "type": "event_deleted", "data": {"id": event_to_delete.id, "title": event_to_delete.title}} diff --git a/tests/test_calendar_tool_tz.py b/tests/test_calendar_tool_tz.py index 71b0e24..fa47fcb 100644 --- a/tests/test_calendar_tool_tz.py +++ b/tests/test_calendar_tool_tz.py @@ -577,6 +577,188 @@ async def test_update_event_expected_weekday_mismatch_rejects(): assert update_called is False +# ── Multi-match disambiguation (Fable #161) ────────────────────────────────── + + +@pytest.mark.asyncio +async def test_update_event_refuses_ambiguous_query_with_candidates(): + """The reported failure: model called update_event(query='Appointment') + when two events matched. Pre-fix: silently mutated matches[0] (the + wrong event). Post-fix: returns success=False with a candidates list + so the model can pick the right one.""" + from fabledassistant.services.tools.calendar import update_event_tool + + # Two events both matching "Appointment" + ev_a = AsyncMock() + ev_a.id = 2 + ev_a.title = "Appointment" + ev_a.start_dt = __import__("datetime").datetime(2026, 4, 30, 12, 0, tzinfo=timezone.utc) + ev_a.location = "" + ev_b = AsyncMock() + ev_b.id = 15 + ev_b.title = "Appointment" + ev_b.start_dt = __import__("datetime").datetime(2026, 5, 1, 12, 0, tzinfo=timezone.utc) + ev_b.location = "Dentist" + + update_called = False + + async def fake_find(*, user_id, query): + return [ev_a, ev_b] + + async def fake_update(*, user_id, event_id, **fields): + nonlocal update_called + update_called = True + return AsyncMock() + + with patch( + "fabledassistant.services.tools.calendar.find_events_by_query", + side_effect=fake_find, + ), patch( + "fabledassistant.services.tools.calendar.events_update_event", + side_effect=fake_update, + ): + result = await update_event_tool( + user_id=1, + arguments={"query": "Appointment", "title": "Dentist"}, + ) + + assert result["success"] is False + assert "Found 2 events" in result["error"] + assert "event_id" in result["error"].lower() + assert "candidates" in result + candidate_ids = [c["id"] for c in result["candidates"]] + assert candidate_ids == [2, 15] + # Critical: nothing was mutated. + assert update_called is False + + +@pytest.mark.asyncio +async def test_update_event_with_event_id_skips_query_lookup(): + """Once the model picks a candidate via event_id, the call proceeds + against that exact event — no query, no ambiguity.""" + from fabledassistant.services.tools.calendar import update_event_tool + + # find_events_by_query should NOT be called when event_id is supplied + find_called = False + + async def fake_find(*, user_id, query): + nonlocal find_called + find_called = True + return [] + + target = AsyncMock() + target.id = 15 + target.title = "Appointment" + target.start_dt = __import__("datetime").datetime(2026, 5, 1, 12, 0, tzinfo=timezone.utc) + target.location = "" + + async def fake_get_event(*, user_id, event_id): + return target if event_id == 15 else None + + captured = {} + + async def fake_update(*, user_id, event_id, **fields): + captured["event_id"] = event_id + captured.update(fields) + ev = AsyncMock() + ev.to_dict.return_value = {"id": event_id, **fields} + return ev + + with patch( + "fabledassistant.services.tools.calendar.find_events_by_query", + side_effect=fake_find, + ), patch( + "fabledassistant.services.events.get_event", + side_effect=fake_get_event, + ), patch( + "fabledassistant.services.tools.calendar.events_update_event", + side_effect=fake_update, + ): + result = await update_event_tool( + user_id=1, + arguments={"event_id": 15, "title": "Dentist: Permanent Crown Fitting"}, + ) + + assert result["success"] is True + assert captured["event_id"] == 15 + assert captured["title"] == "Dentist: Permanent Crown Fitting" + assert find_called is False + + +@pytest.mark.asyncio +async def test_update_event_with_event_id_not_found_returns_error(): + """A bad event_id (non-existent) must surface clearly, not silently + fall back to query-based lookup.""" + from fabledassistant.services.tools.calendar import update_event_tool + + async def fake_get_event(*, user_id, event_id): + return None + + with patch( + "fabledassistant.services.events.get_event", + side_effect=fake_get_event, + ): + result = await update_event_tool( + user_id=1, + arguments={"event_id": 999, "title": "anything"}, + ) + + assert result["success"] is False + assert "999" in result["error"] + assert "No event found" in result["error"] + + +@pytest.mark.asyncio +async def test_update_event_neither_query_nor_event_id_returns_error(): + """Calling update_event with neither identifier is a usage error.""" + from fabledassistant.services.tools.calendar import update_event_tool + + result = await update_event_tool(user_id=1, arguments={"title": "x"}) + assert result["success"] is False + assert "query or event_id" in result["error"] + + +@pytest.mark.asyncio +async def test_delete_event_refuses_ambiguous_query_with_candidates(): + """delete_event must enforce the same disambiguation. Costly to get + wrong — silent matches[0] would delete the wrong event entirely.""" + from fabledassistant.services.tools.calendar import delete_event_tool + + ev_a = AsyncMock() + ev_a.id = 2; ev_a.title = "Appointment" + ev_a.start_dt = __import__("datetime").datetime(2026, 4, 30, 12, 0, tzinfo=timezone.utc) + ev_a.location = "" + ev_b = AsyncMock() + ev_b.id = 15; ev_b.title = "Appointment" + ev_b.start_dt = __import__("datetime").datetime(2026, 5, 1, 12, 0, tzinfo=timezone.utc) + ev_b.location = "Dentist" + + delete_called = False + + async def fake_find(*, user_id, query): + return [ev_a, ev_b] + + async def fake_delete(*, user_id, event_id): + nonlocal delete_called + delete_called = True + + with patch( + "fabledassistant.services.tools.calendar.find_events_by_query", + side_effect=fake_find, + ), patch( + "fabledassistant.services.tools.calendar.events_delete_event", + side_effect=fake_delete, + ): + result = await delete_event_tool( + user_id=1, arguments={"query": "Appointment"}, + ) + + assert result["success"] is False + assert len(result["candidates"]) == 2 + # Most important assertion: nothing was actually deleted. + assert delete_called is False + + @pytest.mark.asyncio async def test_create_event_weekday_check_uses_local_not_utc(): """The weekday check must use the LOCAL date, not the UTC date.