fix: persist assistant tool errors and markdown
This commit is contained in:
@@ -8,6 +8,7 @@ from django.contrib.contenttypes.models import ContentType
|
|||||||
from django.db import models
|
from django.db import models
|
||||||
from django.db.models import Q
|
from django.db.models import Q
|
||||||
from django.utils import timezone
|
from django.utils import timezone
|
||||||
|
from rest_framework.exceptions import PermissionDenied, ValidationError
|
||||||
|
|
||||||
from adventures.models import (
|
from adventures.models import (
|
||||||
Collection,
|
Collection,
|
||||||
@@ -136,6 +137,23 @@ def _parse_float(value):
|
|||||||
return None
|
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):
|
def _serialize_lodging(lodging: Lodging):
|
||||||
return {
|
return {
|
||||||
"id": str(lodging.id),
|
"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(
|
moved_item = next(
|
||||||
(item for item in updated_items if str(item.id) == str(itinerary_item.id)),
|
(item for item in updated_items if str(item.id) == str(itinerary_item.id)),
|
||||||
itinerary_item,
|
itinerary_item,
|
||||||
|
|||||||
@@ -33,6 +33,8 @@ When modifying itineraries:
|
|||||||
When chat context includes a trip collection:
|
When chat context includes a trip collection:
|
||||||
- Treat context as itinerary-wide (potentially multiple stops), not a single destination
|
- 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 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
|
- 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
|
- 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
|
||||||
|
|
||||||
|
|||||||
@@ -8,6 +8,7 @@ import requests as _requests
|
|||||||
from django.contrib.auth import get_user_model
|
from django.contrib.auth import get_user_model
|
||||||
from django.contrib.contenttypes.models import ContentType
|
from django.contrib.contenttypes.models import ContentType
|
||||||
from django.test import TestCase
|
from django.test import TestCase
|
||||||
|
from rest_framework.exceptions import PermissionDenied, ValidationError
|
||||||
from rest_framework.test import APITransactionTestCase
|
from rest_framework.test import APITransactionTestCase
|
||||||
|
|
||||||
from adventures.models import Collection, CollectionItineraryItem, Location, Visit
|
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(remove_result, {"error": "Trip not found"})
|
||||||
self.assertEqual(update_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):
|
class ChatViewSetToolValidationBoundaryTests(TestCase):
|
||||||
def test_trip_context_destination_summary_normalizes_to_first_segment(self):
|
def test_trip_context_destination_summary_normalizes_to_first_segment(self):
|
||||||
@@ -589,6 +647,70 @@ class ChatViewSetToolValidationBoundaryTests(TestCase):
|
|||||||
|
|
||||||
|
|
||||||
class ChatViewSetSearchPlacesClarificationTests(APITransactionTestCase):
|
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.execute_tool")
|
||||||
@patch("chat.views.stream_chat_completion")
|
@patch("chat.views.stream_chat_completion")
|
||||||
@patch("integrations.utils.auto_profile.update_auto_preference_profile")
|
@patch("integrations.utils.auto_profile.update_auto_preference_profile")
|
||||||
@@ -1073,6 +1195,56 @@ class ChatViewSetSearchPlacesClarificationTests(APITransactionTestCase):
|
|||||||
|
|
||||||
|
|
||||||
class ChatViewSetToolExecutionFailureLoopTests(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.execute_tool")
|
||||||
@patch("chat.views.stream_chat_completion")
|
@patch("chat.views.stream_chat_completion")
|
||||||
@patch("integrations.utils.auto_profile.update_auto_preference_profile")
|
@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_stream_chat_completion.call_count, 3)
|
||||||
self.assertEqual(mock_execute_tool.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.execute_tool")
|
||||||
@patch("chat.views.stream_chat_completion")
|
@patch("chat.views.stream_chat_completion")
|
||||||
@patch("integrations.utils.auto_profile.update_auto_preference_profile")
|
@patch("integrations.utils.auto_profile.update_auto_preference_profile")
|
||||||
|
|||||||
@@ -493,6 +493,13 @@ class ChatViewSet(viewsets.ModelViewSet):
|
|||||||
|
|
||||||
context_parts = []
|
context_parts = []
|
||||||
itinerary_stops = []
|
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:
|
if collection_name:
|
||||||
context_parts.append(f"Trip: {collection_name}")
|
context_parts.append(f"Trip: {collection_name}")
|
||||||
if start_date and end_date:
|
if start_date and end_date:
|
||||||
@@ -513,10 +520,8 @@ class ChatViewSet(viewsets.ModelViewSet):
|
|||||||
pass
|
pass
|
||||||
|
|
||||||
if collection:
|
if collection:
|
||||||
context_parts.append(
|
collection_id_guidance = collection_tool_id_guidance
|
||||||
"Collection UUID (use this exact collection_id for get_trip_details and add_to_itinerary): "
|
context_parts.append(f"Collection UUID: {collection.id}")
|
||||||
f"{collection.id}"
|
|
||||||
)
|
|
||||||
seen_stops = set()
|
seen_stops = set()
|
||||||
for location in collection.locations.select_related(
|
for location in collection.locations.select_related(
|
||||||
"city", "country"
|
"city", "country"
|
||||||
@@ -577,6 +582,10 @@ class ChatViewSet(viewsets.ModelViewSet):
|
|||||||
system_prompt = get_system_prompt(request.user, collection)
|
system_prompt = get_system_prompt(request.user, collection)
|
||||||
if context_parts:
|
if context_parts:
|
||||||
system_prompt += "\n\n## Trip Context\n" + "\n".join(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(
|
ChatMessage.objects.create(
|
||||||
conversation=conversation,
|
conversation=conversation,
|
||||||
@@ -883,6 +892,18 @@ class ChatViewSet(viewsets.ModelViewSet):
|
|||||||
function_name,
|
function_name,
|
||||||
result,
|
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 f"data: {json.dumps(error_event)}\n\n"
|
||||||
yield "data: [DONE]\n\n"
|
yield "data: [DONE]\n\n"
|
||||||
return
|
return
|
||||||
|
|||||||
@@ -2,6 +2,8 @@
|
|||||||
import { createEventDispatcher, onMount } from 'svelte';
|
import { createEventDispatcher, onMount } from 'svelte';
|
||||||
import { t } from 'svelte-i18n';
|
import { t } from 'svelte-i18n';
|
||||||
import { mdiSend, mdiPlus, mdiDelete, mdiMenu, mdiClose } from '@mdi/js';
|
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 type { ChatProviderCatalogEntry, CollectionItineraryItem, Location } from '$lib/types.js';
|
||||||
import { addToast } from '$lib/toasts';
|
import { addToast } from '$lib/toasts';
|
||||||
|
|
||||||
@@ -932,6 +934,14 @@
|
|||||||
text: `${result.name.replaceAll('_', ' ')} completed.`
|
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));
|
||||||
|
}
|
||||||
</script>
|
</script>
|
||||||
|
|
||||||
<div
|
<div
|
||||||
@@ -1126,7 +1136,13 @@
|
|||||||
? 'chat-bubble-primary'
|
? 'chat-bubble-primary'
|
||||||
: 'chat-bubble-neutral'}"
|
: 'chat-bubble-neutral'}"
|
||||||
>
|
>
|
||||||
<div class="whitespace-pre-wrap">{msg.content}</div>
|
{#if msg.role === 'assistant'}
|
||||||
|
<div class="assistant-markdown">
|
||||||
|
{@html renderAssistantMarkdown(msg.content)}
|
||||||
|
</div>
|
||||||
|
{:else}
|
||||||
|
<div class="whitespace-pre-wrap break-words">{msg.content}</div>
|
||||||
|
{/if}
|
||||||
{#if msg.role === 'assistant' && msg.tool_results}
|
{#if msg.role === 'assistant' && msg.tool_results}
|
||||||
<div class="mt-2 space-y-2">
|
<div class="mt-2 space-y-2">
|
||||||
{#each deduplicateContextTools(uniqueToolResultsByCallId(msg.tool_results)) as result}
|
{#each deduplicateContextTools(uniqueToolResultsByCallId(msg.tool_results)) as result}
|
||||||
@@ -1327,3 +1343,56 @@
|
|||||||
</div>
|
</div>
|
||||||
</div>
|
</div>
|
||||||
{/if}
|
{/if}
|
||||||
|
|
||||||
|
<style>
|
||||||
|
.assistant-markdown {
|
||||||
|
color: inherit;
|
||||||
|
line-height: 1.5;
|
||||||
|
word-break: break-word;
|
||||||
|
}
|
||||||
|
|
||||||
|
.assistant-markdown :global(p) {
|
||||||
|
margin: 0;
|
||||||
|
}
|
||||||
|
|
||||||
|
.assistant-markdown :global(p + p),
|
||||||
|
.assistant-markdown :global(ul + p),
|
||||||
|
.assistant-markdown :global(ol + p),
|
||||||
|
.assistant-markdown :global(pre + p),
|
||||||
|
.assistant-markdown :global(blockquote + p) {
|
||||||
|
margin-top: 0.5rem;
|
||||||
|
}
|
||||||
|
|
||||||
|
.assistant-markdown :global(ul),
|
||||||
|
.assistant-markdown :global(ol) {
|
||||||
|
margin: 0.5rem 0 0;
|
||||||
|
padding-left: 1.25rem;
|
||||||
|
}
|
||||||
|
|
||||||
|
.assistant-markdown :global(li + li) {
|
||||||
|
margin-top: 0.25rem;
|
||||||
|
}
|
||||||
|
|
||||||
|
.assistant-markdown :global(strong),
|
||||||
|
.assistant-markdown :global(em),
|
||||||
|
.assistant-markdown :global(code),
|
||||||
|
.assistant-markdown :global(a) {
|
||||||
|
color: inherit;
|
||||||
|
}
|
||||||
|
|
||||||
|
.assistant-markdown :global(a) {
|
||||||
|
text-decoration: underline;
|
||||||
|
}
|
||||||
|
|
||||||
|
.assistant-markdown :global(pre) {
|
||||||
|
margin-top: 0.5rem;
|
||||||
|
padding: 0.5rem;
|
||||||
|
border-radius: 0.5rem;
|
||||||
|
background-color: color-mix(in srgb, currentColor 12%, transparent);
|
||||||
|
overflow-x: auto;
|
||||||
|
}
|
||||||
|
|
||||||
|
.assistant-markdown :global(code) {
|
||||||
|
font-size: 0.875em;
|
||||||
|
}
|
||||||
|
</style>
|
||||||
|
|||||||
Reference in New Issue
Block a user