From 84384df2365fd02f91105be650887984e28014ac Mon Sep 17 00:00:00 2001 From: alex Date: Tue, 10 Mar 2026 16:26:02 +0000 Subject: [PATCH] fix(chat): clarify missing-location search requests --- .cursorrules | 72 -------------------- AGENTS.md | 4 +- CLAUDE.md | 98 --------------------------- backend/server/chat/tests.py | 96 ++++++++++++++++++++++++++ backend/server/chat/views/__init__.py | 49 ++++++++++++++ 5 files changed, 147 insertions(+), 172 deletions(-) delete mode 100644 .cursorrules delete mode 100644 CLAUDE.md diff --git a/.cursorrules b/.cursorrules deleted file mode 100644 index b39f74b7..00000000 --- a/.cursorrules +++ /dev/null @@ -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. diff --git a/AGENTS.md b/AGENTS.md index 4d81ce66..f0822b25 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -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. diff --git a/CLAUDE.md b/CLAUDE.md deleted file mode 100644 index 768c63d1..00000000 --- a/CLAUDE.md +++ /dev/null @@ -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. diff --git a/backend/server/chat/tests.py b/backend/server/chat/tests.py index e4b42c8b..c9c19305 100644 --- a/backend/server/chat/tests.py +++ b/backend/server/chat/tests.py @@ -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()) diff --git a/backend/server/chat/views/__init__.py b/backend/server/chat/views/__init__.py index a64c59b7..07a23791 100644 --- a/backend/server/chat/views/__init__.py +++ b/backend/server/chat/views/__init__.py @@ -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,