From d1704af82765c01d2ccd88760533bf5ab628694e Mon Sep 17 00:00:00 2001 From: alex wiesner Date: Sat, 14 Mar 2026 11:22:14 +0000 Subject: [PATCH] fix: persist assistant tool errors and markdown --- backend/server/chat/agent_tools.py | 36 +++- backend/server/chat/llm_client.py | 2 + backend/server/chat/tests.py | 180 ++++++++++++++++++ backend/server/chat/views/__init__.py | 29 ++- .../src/lib/components/AITravelChat.svelte | 71 ++++++- 5 files changed, 312 insertions(+), 6 deletions(-) diff --git a/backend/server/chat/agent_tools.py b/backend/server/chat/agent_tools.py index e73ff8a2..992b949e 100644 --- a/backend/server/chat/agent_tools.py +++ b/backend/server/chat/agent_tools.py @@ -8,6 +8,7 @@ from django.contrib.contenttypes.models import ContentType from django.db import models from django.db.models import Q from django.utils import timezone +from rest_framework.exceptions import PermissionDenied, ValidationError from adventures.models import ( Collection, @@ -136,6 +137,23 @@ def _parse_float(value): return None +def _extract_exception_message(exc, fallback_message): + detail = getattr(exc, "detail", None) + if isinstance(detail, dict): + for value in detail.values(): + if isinstance(value, (list, tuple)) and value: + return str(value[0]) + if value: + return str(value) + elif isinstance(detail, (list, tuple)) and detail: + return str(detail[0]) + elif detail: + return str(detail) + + message = str(exc).strip() + return message or fallback_message + + def _serialize_lodging(lodging: Lodging): return { "id": str(lodging.id), @@ -810,7 +828,23 @@ def move_itinerary_item( } ) - updated_items = reorder_itinerary_items(user, updates) + try: + updated_items = reorder_itinerary_items(user, updates) + except ValidationError as exc: + return { + "error": _extract_exception_message( + exc, + "Unable to move itinerary item due to invalid itinerary update", + ) + } + except PermissionDenied as exc: + return { + "error": _extract_exception_message( + exc, + "You do not have permission to modify this itinerary item", + ) + } + moved_item = next( (item for item in updated_items if str(item.id) == str(itinerary_item.id)), itinerary_item, diff --git a/backend/server/chat/llm_client.py b/backend/server/chat/llm_client.py index 09002664..fb2b8fc0 100644 --- a/backend/server/chat/llm_client.py +++ b/backend/server/chat/llm_client.py @@ -33,6 +33,8 @@ When modifying itineraries: When chat context includes a trip collection: - Treat context as itinerary-wide (potentially multiple stops), not a single destination - Use get_trip_details first when you need complete collection context before searching for places +- Use the active collection_id from context for collection-scoped itinerary management tools (get_trip_details, add_to_itinerary, move_itinerary_item, remove_itinerary_item, update_location_details) +- Call get_trip_details before move_itinerary_item, remove_itinerary_item, or update_location_details when exact itinerary/location IDs are required, and never invent IDs - Ground place searches in trip stops and dates from the provided trip context - Only call search_places when you have a concrete, non-empty location string; if location is missing or unclear, ask a clarifying question to obtain it first diff --git a/backend/server/chat/tests.py b/backend/server/chat/tests.py index 018cbe82..bdd79c76 100644 --- a/backend/server/chat/tests.py +++ b/backend/server/chat/tests.py @@ -8,6 +8,7 @@ import requests as _requests from django.contrib.auth import get_user_model from django.contrib.contenttypes.models import ContentType from django.test import TestCase +from rest_framework.exceptions import PermissionDenied, ValidationError from rest_framework.test import APITransactionTestCase from adventures.models import Collection, CollectionItineraryItem, Location, Visit @@ -459,6 +460,63 @@ class ChatAgentToolItineraryManagementTests(TestCase): self.assertEqual(remove_result, {"error": "Trip not found"}) self.assertEqual(update_result, {"error": "Trip not found"}) + @patch("chat.agent_tools.reorder_itinerary_items") + def test_move_itinerary_item_returns_validation_error_from_reorder( + self, + mock_reorder_itinerary_items, + ): + mock_reorder_itinerary_items.side_effect = ValidationError( + { + "items": ( + "Item abc date 2026-05-30 is before collection start date " + "2026-06-01." + ) + } + ) + + self.collection.start_date = date(2026, 6, 1) + self.collection.save(update_fields=["start_date"]) + + result = move_itinerary_item( + self.owner, + collection_id=str(self.collection.id), + itinerary_item_id=str(self.day1_item.id), + date="2026-06-02", + order=0, + ) + + self.assertEqual( + result, + { + "error": ( + "Item abc date 2026-05-30 is before collection start date " + "2026-06-01." + ) + }, + ) + + @patch("chat.agent_tools.reorder_itinerary_items") + def test_move_itinerary_item_returns_permission_error_from_reorder( + self, + mock_reorder_itinerary_items, + ): + mock_reorder_itinerary_items.side_effect = PermissionDenied( + "You do not have permission to modify items in this collection." + ) + + result = move_itinerary_item( + self.owner, + collection_id=str(self.collection.id), + itinerary_item_id=str(self.day1_item.id), + date="2026-06-02", + order=0, + ) + + self.assertEqual( + result, + {"error": "You do not have permission to modify items in this collection."}, + ) + class ChatViewSetToolValidationBoundaryTests(TestCase): def test_trip_context_destination_summary_normalizes_to_first_segment(self): @@ -589,6 +647,70 @@ class ChatViewSetToolValidationBoundaryTests(TestCase): class ChatViewSetSearchPlacesClarificationTests(APITransactionTestCase): + @patch("chat.views.stream_chat_completion") + @patch("integrations.utils.auto_profile.update_auto_preference_profile") + def test_collection_context_includes_uuid_guidance_for_existing_item_tools( + self, + _mock_auto_profile, + mock_stream_chat_completion, + ): + user = User.objects.create_user( + username="chat-context-tool-guidance-user", + email="chat-context-tool-guidance-user@example.com", + password="password123", + ) + self.client.force_authenticate(user=user) + + collection = Collection.objects.create(user=user, name="Paris Trip") + + conversation_response = self.client.post( + "/api/chat/conversations/", + {"title": "Context Tool Guidance Test"}, + format="json", + ) + self.assertEqual(conversation_response.status_code, 201) + conversation_id = conversation_response.json()["id"] + + async def stream_noop(*args, **kwargs): + yield "data: [DONE]\n\n" + + mock_stream_chat_completion.side_effect = stream_noop + + response = self.client.post( + f"/api/chat/conversations/{conversation_id}/send_message/", + { + "message": "Move my first stop to day 2", + "collection_id": str(collection.id), + "collection_name": collection.name, + }, + format="json", + ) + + self.assertEqual(response.status_code, 200) + list(response.streaming_content) + + stream_call_messages = mock_stream_chat_completion.call_args.args[1] + system_message = next( + message for message in stream_call_messages if message["role"] == "system" + ) + normalized_system_content = system_message["content"].lower() + self.assertIn( + ( + "use this exact collection_id for get_trip_details, " + "add_to_itinerary, move_itinerary_item, remove_itinerary_item, " + "and update_location_details" + ), + normalized_system_content, + ) + self.assertIn( + ( + "call get_trip_details first before move_itinerary_item, " + "remove_itinerary_item, or update_location_details when exact IDs " + "are needed" + ).lower(), + normalized_system_content, + ) + @patch("chat.views.execute_tool") @patch("chat.views.stream_chat_completion") @patch("integrations.utils.auto_profile.update_auto_preference_profile") @@ -1073,6 +1195,56 @@ class ChatViewSetSearchPlacesClarificationTests(APITransactionTestCase): class ChatViewSetToolExecutionFailureLoopTests(APITransactionTestCase): + @patch("chat.views.execute_tool") + @patch("chat.views.stream_chat_completion") + @patch("integrations.utils.auto_profile.update_auto_preference_profile") + def test_required_param_terminal_error_is_persisted_as_assistant_message( + self, + _mock_auto_profile, + mock_stream_chat_completion, + mock_execute_tool, + ): + user = User.objects.create_user( + username="chat-required-error-persist-user", + email="chat-required-error-persist-user@example.com", + password="password123", + ) + self.client.force_authenticate(user=user) + + conversation_response = self.client.post( + "/api/chat/conversations/", + {"title": "Required Error Persistence Test"}, + format="json", + ) + self.assertEqual(conversation_response.status_code, 201) + conversation_id = conversation_response.json()["id"] + + async def validation_error_stream(*args, **kwargs): + yield 'data: {"tool_calls": [{"index": 0, "id": "call_move", "type": "function", "function": {"name": "move_itinerary_item", "arguments": "{}"}}]}\n\n' + yield "data: [DONE]\n\n" + + mock_stream_chat_completion.side_effect = validation_error_stream + mock_execute_tool.return_value = { + "error": "collection_id, itinerary_item_id, and date are required" + } + + response = self.client.post( + f"/api/chat/conversations/{conversation_id}/send_message/", + {"message": "Move that stop"}, + format="json", + ) + + self.assertEqual(response.status_code, 200) + list(response.streaming_content) + + conversation = user.chat_conversations.get(id=conversation_id) + self.assertTrue( + conversation.messages.filter( + role="assistant", + content__contains="attempted to call 'move_itinerary_item' without required arguments", + ).exists() + ) + @patch("chat.views.execute_tool") @patch("chat.views.stream_chat_completion") @patch("integrations.utils.auto_profile.update_auto_preference_profile") @@ -1145,6 +1317,14 @@ class ChatViewSetToolExecutionFailureLoopTests(APITransactionTestCase): self.assertEqual(mock_stream_chat_completion.call_count, 3) self.assertEqual(mock_execute_tool.call_count, 3) + conversation = user.chat_conversations.get(id=conversation_id) + self.assertTrue( + conversation.messages.filter( + role="assistant", + content__contains="could not complete 'web_search'", + ).exists() + ) + @patch("chat.views.execute_tool") @patch("chat.views.stream_chat_completion") @patch("integrations.utils.auto_profile.update_auto_preference_profile") diff --git a/backend/server/chat/views/__init__.py b/backend/server/chat/views/__init__.py index fed0718a..d3993ebf 100644 --- a/backend/server/chat/views/__init__.py +++ b/backend/server/chat/views/__init__.py @@ -493,6 +493,13 @@ class ChatViewSet(viewsets.ModelViewSet): context_parts = [] itinerary_stops = [] + collection_id_guidance = "" + collection_tool_id_guidance = ( + "Use this exact collection_id for get_trip_details, add_to_itinerary, " + "move_itinerary_item, remove_itinerary_item, and update_location_details. " + "Call get_trip_details first before move_itinerary_item, " + "remove_itinerary_item, or update_location_details when exact IDs are needed." + ) if collection_name: context_parts.append(f"Trip: {collection_name}") if start_date and end_date: @@ -513,10 +520,8 @@ class ChatViewSet(viewsets.ModelViewSet): pass if collection: - context_parts.append( - "Collection UUID (use this exact collection_id for get_trip_details and add_to_itinerary): " - f"{collection.id}" - ) + collection_id_guidance = collection_tool_id_guidance + context_parts.append(f"Collection UUID: {collection.id}") seen_stops = set() for location in collection.locations.select_related( "city", "country" @@ -577,6 +582,10 @@ class ChatViewSet(viewsets.ModelViewSet): system_prompt = get_system_prompt(request.user, collection) if context_parts: system_prompt += "\n\n## Trip Context\n" + "\n".join(context_parts) + if collection_id_guidance: + system_prompt += ( + "\n\n## Collection Tool Guardrails\n" + collection_id_guidance + ) ChatMessage.objects.create( conversation=conversation, @@ -883,6 +892,18 @@ class ChatViewSet(viewsets.ModelViewSet): function_name, result, ) + await sync_to_async( + ChatMessage.objects.create, + thread_sensitive=True, + )( + conversation=conversation, + role="assistant", + content=error_event["error"], + ) + await sync_to_async( + conversation.save, + thread_sensitive=True, + )(update_fields=["updated_at"]) yield f"data: {json.dumps(error_event)}\n\n" yield "data: [DONE]\n\n" return diff --git a/frontend/src/lib/components/AITravelChat.svelte b/frontend/src/lib/components/AITravelChat.svelte index 323beba9..f532262b 100644 --- a/frontend/src/lib/components/AITravelChat.svelte +++ b/frontend/src/lib/components/AITravelChat.svelte @@ -2,6 +2,8 @@ import { createEventDispatcher, onMount } from 'svelte'; import { t } from 'svelte-i18n'; import { mdiSend, mdiPlus, mdiDelete, mdiMenu, mdiClose } from '@mdi/js'; + import { marked } from 'marked'; + import DOMPurify from 'dompurify'; import type { ChatProviderCatalogEntry, CollectionItineraryItem, Location } from '$lib/types.js'; import { addToast } from '$lib/toasts'; @@ -932,6 +934,14 @@ text: `${result.name.replaceAll('_', ' ')} completed.` }; } + + function renderMarkdown(markdown: string): string { + return marked(markdown) as string; + } + + function renderAssistantMarkdown(content: string): string { + return DOMPurify.sanitize(renderMarkdown(content)); + }
-
{msg.content}
+ {#if msg.role === 'assistant'} +
+ {@html renderAssistantMarkdown(msg.content)} +
+ {:else} +
{msg.content}
+ {/if} {#if msg.role === 'assistant' && msg.tool_results}
{#each deduplicateContextTools(uniqueToolResultsByCallId(msg.tool_results)) as result} @@ -1327,3 +1343,56 @@
{/if} + +