fix(chat): clarify missing-location search requests
This commit is contained in:
72
.cursorrules
72
.cursorrules
@@ -1,72 +0,0 @@
|
||||
# Voyage Cursor Rules (local-only, gitignored)
|
||||
|
||||
## Project Summary
|
||||
- Voyage is a self-hosted travel companion app (AdventureLog fork).
|
||||
- Stack: SvelteKit 2 + TypeScript frontend, Django REST Framework backend, PostgreSQL/PostGIS, Memcached, Docker, Bun.
|
||||
|
||||
## Pre-Release Policy
|
||||
Voyage is **pre-release** — not yet in production use. During pre-release:
|
||||
- Architecture-level changes are allowed, including replacing core libraries (e.g. LiteLLM).
|
||||
- Prioritize correctness, simplicity, and maintainability over backward compatibility.
|
||||
- Before launch, this policy must be revisited and tightened for production stability.
|
||||
|
||||
## Architecture Essentials
|
||||
- Frontend never calls Django directly.
|
||||
- Route all API requests through `frontend/src/routes/api/[...path]/+server.ts` (proxy to `http://server:8000`).
|
||||
- Services: `web:8015`, `server:8016`, `db:5432`, `cache` internal.
|
||||
- Auth: session-based (`django-allauth`), CSRF from `/auth/csrf/`, send `X-CSRFToken` for mutating requests.
|
||||
|
||||
## Key Locations
|
||||
- Frontend: `frontend/src/`
|
||||
- Backend: `backend/server/`
|
||||
- Django apps: `adventures/`, `users/`, `worldtravel/`, `integrations/`, `achievements/`, `chat/`
|
||||
- Types: `frontend/src/lib/types.ts`
|
||||
- i18n: `frontend/src/locales/`
|
||||
|
||||
## Commands
|
||||
- Frontend:
|
||||
- `cd frontend && bun run format`
|
||||
- `cd frontend && bun run lint`
|
||||
- `cd frontend && bun run check`
|
||||
- `cd frontend && bun run build`
|
||||
- `cd frontend && bun install`
|
||||
- Backend:
|
||||
- `docker compose exec server python3 manage.py test`
|
||||
- `docker compose exec server python3 manage.py migrate`
|
||||
- Use `uv` for local Python tooling when applicable
|
||||
- Docker:
|
||||
- `docker compose up -d`
|
||||
- `docker compose down`
|
||||
|
||||
## Pre-Commit
|
||||
Run in order: format → lint → check → build.
|
||||
|
||||
## Known Issues
|
||||
- `bun run check`: 0 errors + 6 warnings expected (pre-existing in `CollectionRecommendationView.svelte` + `RegionCard.svelte`).
|
||||
- Backend tests: 6/39 pre-existing failures expected (9 new chat tests all pass).
|
||||
- Docker dev setup may show frontend-backend 500 errors beyond homepage.
|
||||
|
||||
## Conventions
|
||||
- Use `$t('key')` for user-facing strings.
|
||||
- Use DaisyUI semantic colors/classes (`bg-primary`, `text-base-content`).
|
||||
- Chat providers: dynamic catalog from `GET /api/chat/providers/`; configured in `CHAT_PROVIDER_CONFIG`.
|
||||
- Chat model override: dropdown selector fed by `GET /api/chat/providers/{provider}/models/`; per-provider persistence via `localStorage` key `voyage_chat_model_prefs`; backend `send_message` accepts optional `model`.
|
||||
- Chat context: collection chats inject collection UUID + multi-stop itinerary context; system prompt guides `get_trip_details`-first reasoning and confirms only before first `add_to_itinerary`; `search_places` prompt guard requires the LLM to have a concrete location string before calling the tool (asks clarifying question otherwise).
|
||||
- Chat tool output: `role=tool` messages hidden from display; tool outputs render as concise summaries; persisted tool rows reconstructed on reload via `rebuildConversationMessages()`.
|
||||
- Chat errors: `_safe_error_payload()` maps LiteLLM exceptions to sanitized user-safe categories (never raw `exc.message`).
|
||||
- Invalid tool calls (missing required args) are detected and short-circuited with a user-visible error — not replayed into history.
|
||||
- Geocoding: `background_geocode_and_assign()` runs in a thread after Location save; populates `region`, `city`, `country`, and also fills `Location.location` from reverse geocode `display_name` (truncated to field max_length) if blank or different.
|
||||
- Chat agent tools (`get_trip_details`, `add_to_itinerary`) respect collection sharing — both owners and `shared_with` members can use them; `list_trips` remains owner-only.
|
||||
- Do **not** attempt to fix known test/config issues during feature work.
|
||||
- Commit and merge completed feature branches promptly once validation passes (avoid leaving finished work unmerged).
|
||||
|
||||
## .memory Files
|
||||
- At the start of any task, read `.memory/manifest.yaml` to discover available files, then read `system.md` and relevant `knowledge/` files for project context.
|
||||
- Read `.memory/decisions.md` for architectural decisions and review verdicts.
|
||||
- Check `.memory/plans/` and `.memory/research/` for prior work on related topics.
|
||||
- These files capture decisions, review verdicts, security findings, and plans from prior sessions.
|
||||
- Do **not** duplicate this info into code comments — `.memory/` is the source of truth for project history.
|
||||
|
||||
## Instruction File Sync
|
||||
- `AGENTS.md`, `CLAUDE.md`, `.cursorrules`, and the Copilot CLI custom instructions must always be kept in sync.
|
||||
- Whenever any one is updated, apply the equivalent change to all the others.
|
||||
@@ -86,5 +86,5 @@ Run in this order:
|
||||
- Do **not** duplicate information from `.memory/` into code comments — keep `.memory/` as the single source of truth for project history.
|
||||
|
||||
## Instruction File Sync
|
||||
- `AGENTS.md` (OpenCode), `CLAUDE.md` (Claude Code), `.cursorrules` (Cursor), and the Copilot CLI custom instructions must always be kept in sync.
|
||||
- Whenever any of these files is updated (new convention, new decision, new workflow rule), apply the equivalent change to all the others.
|
||||
- `AGENTS.md` is the single source of truth for repository instructions.
|
||||
- Do not maintain mirrored instruction files for other tools in this repo.
|
||||
|
||||
98
CLAUDE.md
98
CLAUDE.md
@@ -1,98 +0,0 @@
|
||||
# Voyage Development Instructions (Claude Code)
|
||||
|
||||
## Project
|
||||
- **Name**: Voyage
|
||||
- **Purpose**: Build and maintain a self-hosted travel companion web app (fork of AdventureLog).
|
||||
- **Stack**: SvelteKit 2 (TypeScript) frontend · Django REST Framework (Python) backend · PostgreSQL + PostGIS · Memcached · Docker · Bun (frontend package manager)
|
||||
|
||||
## Pre-Release Policy
|
||||
Voyage is **pre-release** — not yet in production use. During pre-release:
|
||||
- Architecture-level changes are allowed, including replacing core libraries (e.g. LiteLLM).
|
||||
- Prioritize correctness, simplicity, and maintainability over backward compatibility.
|
||||
- Before launch, this policy must be revisited and tightened for production stability.
|
||||
|
||||
## Architecture Overview
|
||||
- Use the API proxy pattern: never call Django directly from frontend components.
|
||||
- Route all frontend API calls through `frontend/src/routes/api/[...path]/+server.ts`.
|
||||
- Proxy target is `http://server:8000`; preserve session cookies and CSRF behavior.
|
||||
- AI chat is embedded in Collections → Recommendations via `AITravelChat.svelte`. There is no standalone `/chat` route. Chat providers are loaded dynamically from `GET /api/chat/providers/` (backed by LiteLLM runtime providers + custom entries like `opencode_zen`). Chat conversations stream via SSE through `/api/chat/conversations/`. Default AI provider/model saved via `UserAISettings` in DB (authoritative over browser localStorage). LiteLLM errors are mapped to sanitized user-safe messages via `_safe_error_payload()` (never exposes raw exception text). Invalid tool calls (missing required args) are detected and short-circuited with a user-visible error — not replayed into history. Chat agent tools (`get_trip_details`, `add_to_itinerary`) respect collection sharing — both owners and `shared_with` members can use them; `list_trips` remains owner-only.
|
||||
- Service ports:
|
||||
- `web` → `:8015`
|
||||
- `server` → `:8016`
|
||||
- `db` → `:5432`
|
||||
- `cache` → internal only
|
||||
- Keep authentication session-based with `django-allauth`.
|
||||
- Fetch CSRF token from `/auth/csrf/` and send `X-CSRFToken` on mutating requests.
|
||||
- Preserve mobile middleware support for `X-Session-Token`.
|
||||
|
||||
## Codebase Layout
|
||||
- Backend root: `backend/server/`
|
||||
- Apps: `adventures/`, `users/`, `worldtravel/`, `integrations/`, `achievements/`, `chat/`
|
||||
- Chat provider config: `backend/server/chat/llm_client.py` (`CHAT_PROVIDER_CONFIG`)
|
||||
- Frontend root: `frontend/src/`
|
||||
- Routes: `src/routes/`
|
||||
- Shared types: `src/lib/types.ts` (includes `ChatProviderCatalogEntry`)
|
||||
- Components: `src/lib/components/` (includes `AITravelChat.svelte`)
|
||||
- Locales: `src/locales/`
|
||||
|
||||
## Development Workflow
|
||||
- Develop Docker-first. Start services with Docker before backend-dependent work.
|
||||
- Use these commands:
|
||||
|
||||
### Frontend (prefer Bun)
|
||||
- `cd frontend && bun run format`
|
||||
- `cd frontend && bun run lint`
|
||||
- `cd frontend && bun run check`
|
||||
- `cd frontend && bun run build`
|
||||
- `cd frontend && bun install`
|
||||
|
||||
### Backend (Docker required; prefer uv for local Python tooling)
|
||||
- `docker compose exec server python3 manage.py test`
|
||||
- `docker compose exec server python3 manage.py migrate`
|
||||
|
||||
### Docker
|
||||
- `docker compose up -d`
|
||||
- `docker compose down`
|
||||
|
||||
## Pre-Commit Checklist
|
||||
Run in this exact order:
|
||||
1. `cd frontend && bun run format`
|
||||
2. `cd frontend && bun run lint`
|
||||
3. `cd frontend && bun run check`
|
||||
4. `cd frontend && bun run build`
|
||||
|
||||
**ALWAYS run format before committing.**
|
||||
|
||||
## Known Issues (Expected)
|
||||
- Frontend `bun run check`: **0 errors + 6 warnings** expected (pre-existing in `CollectionRecommendationView.svelte` + `RegionCard.svelte`)
|
||||
- Backend tests: **6/39 fail** (pre-existing: 2 user email key errors + 4 geocoding API mocks; 9 new chat tests all pass)
|
||||
- Docker dev setup has frontend-backend communication issues (500 errors beyond homepage)
|
||||
|
||||
## Key Patterns
|
||||
- i18n: wrap user-facing strings with `$t('key')`
|
||||
- API access: always use proxy route `/api/[...path]/+server.ts`
|
||||
- Styling: prefer DaisyUI semantic classes (`bg-primary`, `text-base-content`)
|
||||
- CSRF handling: use `/auth/csrf/` + `X-CSRFToken`
|
||||
- Chat providers: dynamic catalog from `GET /api/chat/providers/`; configured in `CHAT_PROVIDER_CONFIG`
|
||||
- Chat model override: dropdown selector fed by `GET /api/chat/providers/{provider}/models/`; persisted in `localStorage` key `voyage_chat_model_prefs`; backend accepts optional `model` param in `send_message`
|
||||
- Chat context: collection chats inject collection UUID + multi-stop itinerary context; system prompt guides `get_trip_details`-first reasoning and confirms only before first `add_to_itinerary`; `search_places` prompt guard requires the LLM to have a concrete location string before calling the tool (asks clarifying question otherwise)
|
||||
- Chat tool output: `role=tool` messages hidden from display; tool outputs render as concise summaries; persisted tool rows reconstructed on reload via `rebuildConversationMessages()`
|
||||
- Chat error surfacing: `_safe_error_payload()` maps LiteLLM exceptions to sanitized user-safe categories (never forwards raw `exc.message`)
|
||||
- Invalid tool calls (missing required args) are detected and short-circuited with a user-visible error — not replayed into history
|
||||
- Geocoding: `background_geocode_and_assign()` runs in a thread after Location save; populates `region`, `city`, `country`, and also fills `Location.location` from reverse geocode `display_name` (truncated to field max_length) if blank or different
|
||||
|
||||
## Conventions
|
||||
- Do **not** attempt to fix known test/configuration issues as part of feature work.
|
||||
- Use `bun` for frontend commands, `uv` for local Python tooling where applicable.
|
||||
- Commit and merge completed feature branches promptly once validation passes (avoid leaving finished work unmerged).
|
||||
|
||||
## .memory Files
|
||||
- At the start of any task, read `.memory/manifest.yaml` to discover available files, then read `system.md` and relevant `knowledge/` files for project context.
|
||||
- Read `.memory/decisions.md` for architectural decisions and review verdicts.
|
||||
- Check relevant files in `.memory/plans/` and `.memory/research/` for prior work on related topics.
|
||||
- These files capture architectural decisions, code review verdicts, security findings, and implementation plans from prior sessions.
|
||||
- Do **not** duplicate information from `.memory/` into code comments — keep `.memory/` as the single source of truth for project history.
|
||||
|
||||
## Instruction File Sync
|
||||
- `AGENTS.md` (OpenCode), `CLAUDE.md` (Claude Code), `.cursorrules` (Cursor), and the Copilot CLI custom instructions must always be kept in sync.
|
||||
- Whenever any of these files is updated (new convention, new decision, new workflow rule), apply the equivalent change to all the others.
|
||||
@@ -3,6 +3,7 @@ from unittest.mock import patch
|
||||
|
||||
from django.contrib.auth import get_user_model
|
||||
from django.test import TestCase
|
||||
from rest_framework.test import APITestCase
|
||||
|
||||
from adventures.models import Collection, CollectionItineraryItem
|
||||
from chat.agent_tools import add_to_itinerary, get_trip_details
|
||||
@@ -146,3 +147,98 @@ class ChatViewSetToolValidationBoundaryTests(TestCase):
|
||||
json.dumps({"error": error_text})
|
||||
)
|
||||
)
|
||||
|
||||
def test_search_places_missing_location_error_detected_for_clarification(self):
|
||||
self.assertTrue(
|
||||
ChatViewSet._is_search_places_missing_location_required_error(
|
||||
"search_places",
|
||||
{"error": "location is required"},
|
||||
)
|
||||
)
|
||||
|
||||
def test_non_search_places_required_error_not_detected_for_clarification(self):
|
||||
self.assertFalse(
|
||||
ChatViewSet._is_search_places_missing_location_required_error(
|
||||
"web_search",
|
||||
{"error": "query is required"},
|
||||
)
|
||||
)
|
||||
|
||||
|
||||
class ChatViewSetSearchPlacesClarificationTests(APITestCase):
|
||||
@patch("chat.views.execute_tool")
|
||||
@patch("chat.views.stream_chat_completion")
|
||||
@patch("integrations.utils.auto_profile.update_auto_preference_profile")
|
||||
def test_missing_search_place_location_streams_clarifying_content(
|
||||
self,
|
||||
_mock_auto_profile,
|
||||
mock_stream_chat_completion,
|
||||
mock_execute_tool,
|
||||
):
|
||||
user = User.objects.create_user(
|
||||
username="chat-clarify-user",
|
||||
email="chat-clarify-user@example.com",
|
||||
password="password123",
|
||||
)
|
||||
self.client.force_authenticate(user=user)
|
||||
|
||||
conversation_response = self.client.post(
|
||||
"/api/chat/conversations/",
|
||||
{"title": "Clarification Test"},
|
||||
format="json",
|
||||
)
|
||||
self.assertEqual(conversation_response.status_code, 201)
|
||||
conversation_id = conversation_response.json()["id"]
|
||||
|
||||
async def mock_stream(*args, **kwargs):
|
||||
yield 'data: {"tool_calls": [{"index": 0, "id": "call_1", "type": "function", "function": {"name": "search_places", "arguments": "{}"}}]}\n\n'
|
||||
yield "data: [DONE]\n\n"
|
||||
|
||||
mock_stream_chat_completion.side_effect = mock_stream
|
||||
mock_execute_tool.return_value = {"error": "location is required"}
|
||||
|
||||
response = self.client.post(
|
||||
f"/api/chat/conversations/{conversation_id}/send_message/",
|
||||
{"message": "Find good places"},
|
||||
format="json",
|
||||
)
|
||||
|
||||
self.assertEqual(response.status_code, 200)
|
||||
self.assertEqual(response["Content-Type"], "text/event-stream")
|
||||
|
||||
chunks = [
|
||||
chunk.decode("utf-8")
|
||||
if isinstance(chunk, (bytes, bytearray))
|
||||
else str(chunk)
|
||||
for chunk in response.streaming_content
|
||||
]
|
||||
payload_lines = [
|
||||
chunk.strip()[len("data: ") :]
|
||||
for chunk in chunks
|
||||
if chunk.strip().startswith("data: ")
|
||||
]
|
||||
|
||||
done_count = sum(1 for payload in payload_lines if payload == "[DONE]")
|
||||
self.assertEqual(done_count, 1)
|
||||
|
||||
json_payloads = [
|
||||
json.loads(payload) for payload in payload_lines if payload != "[DONE]"
|
||||
]
|
||||
self.assertTrue(any("content" in payload for payload in json_payloads))
|
||||
self.assertFalse(
|
||||
any(payload.get("error_category") for payload in json_payloads)
|
||||
)
|
||||
|
||||
content_payload = next(
|
||||
payload for payload in json_payloads if "content" in payload
|
||||
)
|
||||
self.assertIn("specific location", content_payload["content"].lower())
|
||||
|
||||
clarifying_message = (
|
||||
user.chat_conversations.get(id=conversation_id)
|
||||
.messages.filter(role="assistant")
|
||||
.order_by("created_at")
|
||||
.last()
|
||||
)
|
||||
self.assertIsNotNone(clarifying_message)
|
||||
self.assertIn("specific location", clarifying_message.content.lower())
|
||||
|
||||
@@ -176,6 +176,28 @@ class ChatViewSet(viewsets.ModelViewSet):
|
||||
"error_category": "tool_validation_error",
|
||||
}
|
||||
|
||||
@classmethod
|
||||
def _is_search_places_missing_location_required_error(cls, tool_name, result):
|
||||
if tool_name != "search_places" or not cls._is_required_param_tool_error(
|
||||
result
|
||||
):
|
||||
return False
|
||||
|
||||
error_text = (result or {}).get("error") if isinstance(result, dict) else ""
|
||||
if not isinstance(error_text, str):
|
||||
return False
|
||||
|
||||
normalized_error = error_text.strip().lower()
|
||||
return "location" in normalized_error
|
||||
|
||||
@staticmethod
|
||||
def _build_search_places_location_clarification_message():
|
||||
return (
|
||||
"Could you share the specific location you'd like me to search near "
|
||||
"(city, neighborhood, or address)? I can also focus on food, "
|
||||
"activities, or lodging."
|
||||
)
|
||||
|
||||
@action(detail=True, methods=["post"])
|
||||
def send_message(self, request, pk=None):
|
||||
# Auto-learn preferences from user's travel history
|
||||
@@ -411,6 +433,33 @@ class ChatViewSet(viewsets.ModelViewSet):
|
||||
thread_sensitive=True,
|
||||
)(**tool_message)
|
||||
|
||||
if self._is_search_places_missing_location_required_error(
|
||||
function_name,
|
||||
result,
|
||||
):
|
||||
clarification_content = self._build_search_places_location_clarification_message()
|
||||
await sync_to_async(
|
||||
ChatMessage.objects.create,
|
||||
thread_sensitive=True,
|
||||
)(
|
||||
conversation=conversation,
|
||||
role="assistant",
|
||||
content=clarification_content,
|
||||
)
|
||||
|
||||
await sync_to_async(
|
||||
conversation.save,
|
||||
thread_sensitive=True,
|
||||
)(update_fields=["updated_at"])
|
||||
|
||||
yield (
|
||||
"data: "
|
||||
f"{json.dumps({'content': clarification_content})}"
|
||||
"\n\n"
|
||||
)
|
||||
yield "data: [DONE]\n\n"
|
||||
return
|
||||
|
||||
await sync_to_async(
|
||||
conversation.save,
|
||||
thread_sensitive=True,
|
||||
|
||||
Reference in New Issue
Block a user