fix(events-tools): refuse ambiguous query, require event_id to disambiguate (#161)
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 <noreply@anthropic.com>
This commit is contained in:
@@ -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.
|
||||
|
||||
Reference in New Issue
Block a user