From c918c9ce2f4fe9091376ecab38cff512f7f8cbe7 Mon Sep 17 00:00:00 2001 From: alex Date: Mon, 9 Mar 2026 22:04:53 +0000 Subject: [PATCH] fix(chat): support shared trips and polish controls --- .memory/decisions.md | 20 ++ .memory/manifest.yaml | 4 + .memory/plans/chat-provider-fixes.md | 187 ++++++++++++++++++ .memory/sessions/continuity.md | 16 +- backend/server/chat/agent_tools.py | 10 +- backend/server/chat/tests.py | 148 ++++++++++++++ .../src/lib/components/AITravelChat.svelte | 51 ++++- frontend/src/locales/ar.json | 5 + frontend/src/locales/de.json | 5 + frontend/src/locales/en.json | 5 + frontend/src/locales/es.json | 5 + frontend/src/locales/fr.json | 5 + frontend/src/locales/hu.json | 5 + frontend/src/locales/it.json | 5 + frontend/src/locales/ja.json | 5 + frontend/src/locales/ko.json | 5 + frontend/src/locales/nl.json | 5 + frontend/src/locales/no.json | 5 + frontend/src/locales/pl.json | 5 + frontend/src/locales/pt-br.json | 5 + frontend/src/locales/ro.json | 5 + frontend/src/locales/ru.json | 5 + frontend/src/locales/sk.json | 5 + frontend/src/locales/sv.json | 5 + frontend/src/locales/tr.json | 5 + frontend/src/locales/uk.json | 5 + frontend/src/locales/zh.json | 5 + 27 files changed, 521 insertions(+), 15 deletions(-) create mode 100644 backend/server/chat/tests.py diff --git a/.memory/decisions.md b/.memory/decisions.md index bbf1425b..d2fe7df5 100644 --- a/.memory/decisions.md +++ b/.memory/decisions.md @@ -359,6 +359,16 @@ - **Reference**: See [Plan: Chat provider fixes](plans/chat-provider-fixes.md#chat-tool-output-cleanup), original CRITICAL at decisions.md:262-267 - **Date**: 2026-03-09 +## Correctness Review: chat-regression-tests +- **Verdict**: APPROVED (score 0) +- **Lens**: Correctness +- **Scope**: `backend/server/chat/tests.py` — shared-trip tool access regressions (owner/shared-member/non-member for `get_trip_details` and `add_to_itinerary`) and required-param regex boundary tests (`_is_required_param_tool_error`, `_is_required_param_tool_error_message_content`). +- **Acceptance criteria**: All 4 verified — tests match current source (correct mock targets, return shapes, error strings), shared-trip access regression fully covered, regex boundaries covered for both gap-closure and false-positive-prevention cases, no new test infrastructure dependencies. +- **No defects found**. Tests are behavior-focused (call actual tool functions, assert on documented return contracts) without coupling to implementation internals. +- **Prior findings confirmed**: Shared-trip fix at `agent_tools.py:326,464-466` (plans/chat-provider-fixes.md:404-407) matches test expectations. Regex boundaries match source at `views/__init__.py:135-153` and error strings at `agent_tools.py:401-403,603-607`. Prior known gap (`dates must be a non-empty list` bypassing regex, decisions.md:166) confirmed resolved by `"dates is required"` change. +- **Reference**: See [Plan: Chat provider fixes](plans/chat-provider-fixes.md#review-verdict--chat-regression-tests) +- **Date**: 2026-03-09 + ## Tester Validation: embedded-chat-ux-polish - **Status**: PASS (Both Standard + Adversarial passes) - **Scope**: Sidebar default closed for embedded mode, compact header with settings dropdown, bounded height, chip scroll behavior, streaming indicator visibility. @@ -372,3 +382,13 @@ - **Residual**: Two low-priority follow-ups (aria-label i18n, dropdown outside-click behavior) — not blocking. - **Reference**: See [Plan: Chat provider fixes](plans/chat-provider-fixes.md#tester-validation--embedded-chat-ux-polish) - **Date**: 2026-03-09 + +## Re-Review: shared-trip-tool-access — .distinct() fix +- **Verdict**: APPROVED (score 0) +- **Lens**: Correctness (targeted re-review) +- **Scope**: `.distinct()` addition to shared-aware collection lookups in `agent_tools.py` and owner-in-shared_with regression tests in `tests.py`. +- **Original finding status**: **RESOLVED**. Both `get_trip_details` (line 327) and `add_to_itinerary` (line 467) now chain `.distinct()` after `Q(user=user) | Q(shared_with=user)` filter, preventing `MultipleObjectsReturned` when owner is also in `shared_with`. Pattern matches established codebase convention (`adventures/mcp.py:41`, `collection_view.py:174-175`). +- **Regression tests verified**: `test_get_trip_details_owner_also_in_shared_with_avoids_duplicates` (tests.py:53-59) and `test_add_to_itinerary_owner_also_in_shared_with_avoids_duplicates` (tests.py:81-96) both add owner to `shared_with` and exercise the exact codepath that would raise `MultipleObjectsReturned` without `.distinct()`. +- **No new issues introduced**: `.distinct()` placement in ORM chain is correct, no logic changes to error handling or return shapes, no mutations to other code paths. +- **Reference**: See [Plan: Chat provider fixes](plans/chat-provider-fixes.md#shared-trip-tool-access) +- **Date**: 2026-03-09 diff --git a/.memory/manifest.yaml b/.memory/manifest.yaml index b35db099..056d1f2f 100644 --- a/.memory/manifest.yaml +++ b/.memory/manifest.yaml @@ -77,6 +77,10 @@ categories: description: "Chat provider fixes plan (COMPLETE) — chat-loop-hardening, default-ai-settings, suggestion-add-flow, chat-tool-grounding-and-confirmation, chat-tool-output-cleanup, embedded-chat-ux-polish workstreams with full review/test records" group: plans + - path: research/chat-regression-coverage-gaps.md + description: Identified test coverage gaps for chat fixes — get_trip_details shared access, tool short-circuit, UI output cleanup + group: research + # Deprecated (content migrated) - path: knowledge.md description: "DEPRECATED — migrated to knowledge/ nested structure. See knowledge/ files." diff --git a/.memory/plans/chat-provider-fixes.md b/.memory/plans/chat-provider-fixes.md index 3a2a280f..b11419e7 100644 --- a/.memory/plans/chat-provider-fixes.md +++ b/.memory/plans/chat-provider-fixes.md @@ -394,3 +394,190 @@ In `selectConversation()`, after loading `data.messages`, reconstruct `tool_resu **COVERAGE: N/A** — No automated test suite for `chat` app. All validation via in-container regex checks + lead's live-run evidence. Recommended follow-up: add Django TestCase for (a) UUID context injection with authorized vs unauthorized collection_id, (b) DoesNotExist path does not trigger short-circuit, (c) empty dates triggers short-circuit. **Non-blocking known issues (accepted, pre-existing):** `get_trip_details` DoesNotExist wording semantically ambiguous (reviewer WARNING); `get_trip_details` excludes shared-collection members from `filter(user=user)` — both pre-existing, not introduced by this feature. + +## Completion Note — `chat-a11y-and-dropdown-polish` (2026-03-09) + +- Replaced embedded chat header hardcoded aria labels with i18n keys (`chat_a11y.show_conversations_aria`, `chat_a11y.hide_conversations_aria`, `chat_a11y.ai_settings_aria`) in `AITravelChat.svelte`. +- Added `chat_a11y` key group to all locale JSON files to keep key parity. +- Added settings dropdown close behavior on outside interaction (`pointerdown`/`mousedown`/`touchstart`) and `Escape`, with mount-time listener cleanup mirroring the existing dropdown pattern used elsewhere. + +## Review Verdict — `chat-a11y-and-dropdown-polish` (2026-03-09) + +### STATUS: APPROVED (score 3) + +**Lens**: Correctness + +**Checked and confirmed safe:** +- All hardcoded English `aria-label` strings replaced with `$t('chat_a11y.*')` calls (lines 778–781, 810). Zero raw strings remain. +- All 20 locale JSON files have key parity: `chat_a11y.{show_conversations_aria, hide_conversations_aria, ai_settings_aria}` present in each (English placeholders — correct for fallback, content translation is a separate concern). +- Outside-click listeners (`pointerdown`/`mousedown`/`touchstart`) correctly close dropdown via `settingsDropdownRef.contains(target)` check + `bind:open` two-way sync with native `
`. +- Escape handler closes dropdown on `keydown` Escape. +- `onMount` cleanup function removes all 4 listeners (3 outside-click + 1 keydown) using the same references. No leak. +- `
` bidirectional binding ensures no conflict between native summary toggle and programmatic close. +- No interaction regression with textarea `handleKeydown` (Enter-only) or date-selector modal. + +**WARNING (1):** Escape handler (line 106) fires on every Escape keypress globally, even when `settingsOpen` is already `false`. No functional bug (idempotent assignment) but lacks a `settingsOpen` guard. (confidence: LOW) + +**SUGGESTIONS (2):** +1. Non-English locale `chat_a11y` values are English placeholders — track for human translation. +2. `outsideEvents` includes both `pointerdown` and `mousedown` — handler fires twice per click on most browsers (second is no-op). Using only `pointerdown` + `touchstart` would be cleaner. + +**Closes prior residual items** from `embedded-chat-ux-polish` tester validation: both the hardcoded aria-label i18n issue and the dropdown outside-click behavior are now resolved. + +## Tester Validation — `chat-a11y-and-dropdown-polish` (2026-03-09) + +### STATUS: PASS + +**Lead evidence accepted:** +- `bun run format`, `bun run lint`, `bun run check` (0 errors, 6 pre-existing warnings), `bun run build` all passed. +- Reviewer APPROVED (score 3); two low-priority SUGGESTIONS (non-English locale placeholders + `pointerdown`/`mousedown` double-registration) confirmed non-blocking. + +**Backend test suite:** `docker compose exec server python3 manage.py test chat --keepdb` — **9/9 PASS**. Zero regressions. + +**Standard pass findings (code inspection + live browser):** + +- AC1 (i18n aria-labels): ✅ All three hardcoded English strings replaced with `$t('chat_a11y.*')` calls (lines 778–781, 812). Live DOM confirmed: `allAriaLabels.filter(key.startsWith('chat_a11y'))` returned `[]` — no raw key strings leaked. +- AC2 (locale key parity): ✅ `chat_a11y` group present in all 20 locale JSON files confirmed. English values are real strings; non-English locales use English placeholders (accepted — translation is out of scope). +- AC3 (Escape closes dropdown): ✅ Live browser: `settingsDetails.open === true` before Escape, `false` after `keyboard.press('Escape')`. +- AC4 (outside click closes dropdown): ✅ Live browser: click on header h3 (outside `settingsDropdownRef.contains(target)`) → `settingsDetails.open === false`. +- AC5 (sidebar toggle aria updates): ✅ Before click: `aria-label="Show conversations"`, `aria-expanded="false"`. After click: `aria-label="Hide conversations"`, `[expanded]` state confirmed in accessibility snapshot. Sidebar conversation list rendered correctly. +- AC6 (listener cleanup): ✅ `onMount` return removes all 4 listeners using same references. No leak. +- AC7 (`bind:open` bidirectionality): ✅ `
` at line 807. `aria-expanded={settingsOpen}` on summary tracks in sync. + +**Screenshot evidence:** Captured embedded chat header with Conversations sidebar open, hamburger (×) button and gear icon visible, "Travel Assistant · test" title, Recommendations tab active. Screenshot deleted post-verification (saved to MCP temp dir, not git-tracked). + +**Adversarial pass findings:** + +1. **Hypothesis: `pointerdown`+`mousedown` double-fire causes open→close→open flicker.** Synthesized both events on outside element after Svelte-bound open. Observed: `open_after_outside_events: false`, `double_fire_safe: true`. Second handler fires on already-false value — idempotent. **Not vulnerable.** + +2. **Hypothesis: raw i18n key strings leak to DOM on locale fallback.** All `[aria-label]` values checked for `chat_a11y` prefix. Zero raw keys found. svelte-i18n falls back to English strings, not key names. **Not vulnerable.** + +3. **Hypothesis: Escape key fires globally when dropdown already closed (reviewer WARNING).** Confirmed true — no `settingsOpen` guard. Assignment `settingsOpen = false` when already `false` is a no-op. **Accepted non-functional defect per reviewer.** + +4. **Hypothesis: `aria-expanded` on `` desyncs when `
` closed by native browser toggle.** `bind:open={settingsOpen}` is bidirectional — native toggle updates `settingsOpen` → `aria-expanded`. **Not vulnerable.** + +5. **Hypothesis: sidebar `aria-controls` target missing on desktop (button is `lg:hidden`).** `id="chat-conversations-sidebar"` is always rendered; `aria-controls` reference always valid. **Not vulnerable.** + +**MUTATION_ESCAPES: 0/4** — All i18n, event handler, aria-sync, and listener-cleanup paths cover mutations. The reviewer WARNING about the missing `settingsOpen` guard is a superficial mutation (no functional impact). + +**FLAKY: 0** + +**COVERAGE: N/A** — No automated frontend test suite. All validation via in-browser DOM evaluation + keyboard/click interaction tests. + +**Residual low-priority items (not blocking):** +- Non-English locale `chat_a11y` values are English placeholders — requires human translation (separate concern). +- `outsideEvents` includes both `pointerdown` and `mousedown` — double-fires but idempotent. Could simplify to `['pointerdown', 'touchstart']`. +- Escape handler lacks `settingsOpen` guard — idempotent no-op, no functional consequence. + +## Completion Note — `shared-trip-tool-access` (2026-03-09) + +- Updated `backend/server/chat/agent_tools.py` so `get_trip_details` and `add_to_itinerary` now authorize collections with the existing shared-access pattern `Q(user=user) | Q(shared_with=user)`. +- Preserved existing owner-only behavior for `list_trips` and kept prior error responses unchanged (`collection_id ... you can access` for trip-details misses, `Trip not found` for itinerary-add misses). +- Follow-up applied: added `.distinct()` to both shared-aware collection lookups to avoid `MultipleObjectsReturned` when an owner is also present in `shared_with`, and added regression tests in `backend/server/chat/tests.py` for that edge case. + +## Completion Note — `chat-regression-tests` (2026-03-09) + +- Added `backend/server/chat/tests.py` with focused backend regressions for shared-trip tool access: owner + shared-member success for `get_trip_details`, shared-member success for `add_to_itinerary`, and non-member denial behavior for both tools. +- Added required-parameter boundary tests against `ChatViewSet._is_required_param_tool_error` and `_is_required_param_tool_error_message_content`, confirming `dates is required` matches while `dates must be a non-empty list` and `collection_id is required and must reference a trip you can access` do not short-circuit. + +## Review Verdict — `chat-regression-tests` (2026-03-09) + +### STATUS: APPROVED (score 0) + +**Lens**: Correctness + +**Acceptance criteria verified**: +- Tests pass with current codebase: all tool function calls and static method invocations match current source signatures and return shapes. Mock targets (`adventures.models.background_geocode_and_assign`) are correct. +- Shared-trip access regression covered: 4 test methods exercise owner access, shared-member access (both get_trip_details and add_to_itinerary), and non-member denial for both tools — covering the `Q(user=user)|Q(shared_with=user)` fix at `agent_tools.py:326,464-466`. +- Required-param regex boundaries covered: 3 boundary tests confirm `"dates is required"` matches (closes prior gap), `"dates must be a non-empty list"` does not match, and `"collection_id is required and must reference a trip you can access"` does not false-positive short-circuit (both `_is_required_param_tool_error` and `_is_required_param_tool_error_message_content`). +- No new test infrastructure: only stdlib, Django TestCase, and existing project models/views imported. No new pip packages or external services. + +**No defects found.** Tests are behavior-focused (call actual tool functions, assert documented return contracts) without overfitting to implementation details. Regex boundary tests use exact production error strings — appropriate since these are stable API-level contracts. + +**SUGGESTIONS**: (1) `test_non_member_access_remains_denied` bundles two independent assertions; splitting would improve diagnostic granularity. (2) Multi-param positive match (`"collection_id, name, latitude, and longitude are required"`) not covered but was validated in prior tester sessions. + +See [decisions.md](../decisions.md#correctness-review-chat-regression-tests). + +## Tester Validation — `shared-trip-tool-access` (2026-03-09) + +### STATUS: PASS + +**Test run:** `docker compose exec server python3 manage.py test chat --keepdb --verbosity=2` — **9/9 PASS** (0 failures, 0 errors). Full baseline: 24/30 Django-wide pass; 6/30 pre-existing failures unchanged. Zero new regressions. + +**Standard pass — code inspection + live test results:** + +- `get_trip_details` (line 326): `Collection.objects.filter(Q(user=user) | Q(shared_with=user)).distinct().get(id=collection_id)` — correct shared-access pattern. `Collection.shared_with` is a `ManyToManyField(User)` (line 288 of `adventures/models.py`), so both predicates are valid FK/M2M joins. `.distinct()` prevents `MultipleObjectsReturned` when the requesting user appears in both `user=user` (owner) and `shared_with=user` (also added as member). +- `add_to_itinerary` (line 466): Identical filter pattern. `.distinct()` applied. The `Location.objects.create(user=user, ...)` at line 471 correctly assigns the location to the shared user's account (not the owner's) — consistent with how the REST API handles shared-user writes (`day_suggestions.py:55` also checks shared membership before allowing writes). +- `list_trips` (line 214): Remains owner-only (`filter(user=user)`) by design — consistent with the feature's accepted scope. This is not a regression. +- `send_message` collection-context gate (views/__init__.py:244-253): Uses same pattern (`owner == user OR shared_with.filter(id=user.id).exists()`). Consistent with tool-layer access. +- Non-member denial: `Collection.DoesNotExist` propagates correctly from `.get()` → caught at lines 402 and 526 → returns appropriate error strings. + +**Test coverage of core acceptance criteria:** + +| Criterion | Test method | Result | +|---|---|---| +| Owner can call `get_trip_details` | `test_get_trip_details_allows_owner_access` | PASS | +| Shared user can call `get_trip_details` | `test_get_trip_details_allows_shared_user_access` | PASS | +| Owner also-in-shared_with doesn't crash | `test_get_trip_details_owner_also_in_shared_with_avoids_duplicates` | PASS | +| Shared user can call `add_to_itinerary` | `test_add_to_itinerary_allows_shared_user_access` | PASS | +| Owner also-in-shared_with add doesn't crash | `test_add_to_itinerary_owner_also_in_shared_with_avoids_duplicates` | PASS | +| Non-member denied for both tools | `test_non_member_access_remains_denied` | PASS | +| DoesNotExist error not false-positive short-circuit | `test_collection_access_error_does_not_short_circuit_required_param_regex` | PASS | +| `dates is required` matches short-circuit | `test_dates_is_required_matches_required_param_short_circuit` | PASS | +| Old "dates must be non-empty list" does not match | `test_dates_non_empty_list_error_does_not_match_required_param_short_circuit` | PASS | + +**Adversarial pass (4 hypotheses):** + +1. **Hypothesis: invalid (non-UUID) `collection_id` causes unhandled exception, not a clean DoesNotExist.** Django UUIDField `.get(id="not-a-uuid")` raises `ValidationError` or `ValueError`, not `DoesNotExist` — the `except Exception` fallback at lines 406-408 catches it and returns `{"error": "An unexpected error occurred while fetching trip details"}`. No crash. **Not vulnerable** (exception absorbed, graceful error). + - _For `add_to_itinerary`_: Same — caught by `except Exception` at lines 528-530. Returns `{"error": "An unexpected error occurred while adding to itinerary"}`. **Not vulnerable.** + +2. **Hypothesis: shared user can overwrite collection membership by being added to `shared_with` of a collection they do not own.** `add_to_itinerary` creates a `Location` with `user=shared_user` and then calls `collection.locations.add(location)` — this adds the new location to the owner's collection. The location's user FK is `shared_user`, which is correct (shared users own their own contributed locations). **No privilege escalation.** + +3. **Hypothesis: race condition — user removed from `shared_with` between filter and write inside `add_to_itinerary`.** Filter + `.get()` runs in a single DB query; the `Collection.DoesNotExist` path fires before any write occurs. No partial write possible. **Not vulnerable** (read-before-write order is safe in this non-transactional case). + +4. **Hypothesis: `list_trips` leaks shared collections to non-owners by exposing collections where `shared_with=user`.** Confirmed: `list_trips` uses `filter(user=user)` only (line 214). Shared collections do not appear in `list_trips` output for shared users. **No information leak; intentionally owner-scoped.** + +**MUTATION_ESCAPES: 1/5** — Invalid UUID input to `get_trip_details`/`add_to_itinerary` falls through to the generic `except Exception` handler rather than `DoesNotExist`, so test `test_non_member_access_remains_denied` would NOT detect a mutation that accidentally drops the `Q(shared_with=user)` clause for malformed UUIDs. However, valid UUID non-member inputs (the primary production scenario) are correctly caught. Risk is very low. + +**FLAKY: 0** + +**COVERAGE: N/A** — No coverage tooling configured. 6 of 9 tests directly exercise the changed lines (326, 466). The `.distinct()` edge case has its own dedicated test methods. + +**LESSON_CHECKS:** +- Prior lesson (chat-tool-grounding-and-confirmation, adversarial item 5): shared member gets UUID in context but `get_trip_details` returns DoesNotExist — **CONTRADICTED / RESOLVED** by this feature. The fix (`Q(user=user) | Q(shared_with=user)`) means shared members now successfully retrieve trip details. The prior finding is no longer valid. +- Prior lesson (pre-existing, pre-release): shared-user write ownership for `add_to_itinerary` sets `user=shared_user` not `user=collection.user` — **confirmed** acceptable, consistent with REST API pattern. + +**Known residual non-issue:** `test_non_member_access_remains_denied` bundles two independent assertions (noted by reviewer SUGGESTION); splitting would improve diagnostic granularity but does not affect correctness of the test outcome. + +## Tester Validation — `chat-regression-tests` (2026-03-09) + +### STATUS: PASS + +**Test run:** `docker compose exec server python3 manage.py test chat --keepdb -v 2` — **9/9 PASS** (independently verified by tester; not just lead-reported). Full Django suite: 39 tests — 33 pass, 6 fail (all 6 pre-existing: 2 user email key errors + 4 geocoding mock failures). Zero new regressions. + +**Standard pass findings:** + +- All 6 `ChatAgentToolSharedTripAccessTests` exercise the `Q(user=user) | Q(shared_with=user)` fix with real DB operations: owner access, shared-member access for both `get_trip_details` and `add_to_itinerary`, `MultipleObjectsReturned` guard (owner also in `shared_with`), and non-member denial. +- All 3 `ChatViewSetToolValidationBoundaryTests` cover the exact boundary cases that are production contracts: `"dates is required"` short-circuits (gap closed by `chat-tool-grounding-and-confirmation`), `"dates must be a non-empty list"` does not short-circuit (regression guard), `"collection_id is required and must reference a trip you can access"` does not false-positive (DoesNotExist variant). Both `_is_required_param_tool_error` and `_is_required_param_tool_error_message_content` paths tested for the DoesNotExist string. +- Mock target `adventures.models.background_geocode_and_assign` is correct. +- Tests are behavior-focused; reviewer confirmed signature and return shape matches current source. + +**Adversarial pass findings (5 hypotheses):** + +1. **Hypothesis: regex accepts trailing text as false positive.** `re.fullmatch` with `r"[a-z0-9_,\-\s]+ (is|are) required"` rejects `"collection_id is required and must reference..."` because trailing text breaks fullmatch. **Confirmed safe; covered by test.** +2. **Hypothesis: `latitude=0.0` treated as falsy, bypassing `add_to_itinerary` guard.** Guard is `latitude is None` (line 460), not truthiness check. `0.0 is None` → False, guard passes. **Not vulnerable — code-inspection confirmed.** +3. **Hypothesis: non-existent UUID raises unhandled exception.** `Collection.DoesNotExist` caught at line 402; invalid UUID string caught by broad `except Exception` at line 406. Both return error dicts, no exception escapes. **Not vulnerable.** +4. **Hypothesis: `"collection_id is required"` positive case absent creates regression blind spot.** This was validated in the `chat-loop-hardening` pass (18 regex cases). New tests target the delta boundary cases only. **Acceptable scope.** +5. **Hypothesis: additional regex adversarial inputs (unicode injection, newline injection, whitespace-only) match unexpectedly.** Verified directly against production function in-container: all return `False`. **Not vulnerable.** + +**MUTATION_ESCAPES: 0/5** — All five mutation checks detected by the executed suite. `fullmatch` boundary tested; `.distinct()` regression tested; shared-member positive path tested; non-member denial tested; both regex helper methods exercised. + +**FLAKY: 0** + +**COVERAGE: N/A** — Backend `chat` app now has 9 tests covering all critical paths for the `shared-trip-tool-access` and regression-test workstreams. No frontend test infrastructure exists (out of scope). + +**LESSON_CHECKS:** +- Prior tester finding (chat-tool-grounding-and-confirmation adversarial item 5): shared-member `get_trip_details` returns DoesNotExist — **CONTRADICTED / RESOLVED** by `shared-trip-tool-access` fix. Confirmed by `test_get_trip_details_allows_shared_user_access` passing in this run. +- Prior tester finding (chat-loop-hardening): `get_weather` "dates must be a non-empty list" did not short-circuit — **RESOLVED** by `chat-tool-grounding-and-confirmation`. Confirmed by `test_dates_is_required_matches_required_param_short_circuit` passing. + +**Reviewer optional suggestions** (not blocking, not addressed): (1) split `test_non_member_access_remains_denied` into two test methods; (2) add explicit multi-param positive regex case. Neither represents a coverage gap for the fixed behavior. diff --git a/.memory/sessions/continuity.md b/.memory/sessions/continuity.md index 6ccaf6d1..059fc8d6 100644 --- a/.memory/sessions/continuity.md +++ b/.memory/sessions/continuity.md @@ -1,12 +1,12 @@ # Session Continuity ## Last Session (2026-03-09) -- Completed `chat-provider-fixes` follow-up round with three additional workstreams: - - `chat-tool-grounding-and-confirmation`: trip context now injects collection UUID for `get_trip_details`/`add_to_itinerary`; system prompt confirms only before first add action; tool error wording aligned with short-circuit regex (`get_weather` gap resolved) - - `chat-tool-output-cleanup`: `role=tool` messages hidden from display; tool outputs render as concise summaries; persisted tool rows reconstructed into `tool_results` on reload - - `embedded-chat-ux-polish`: provider/model selectors in compact settings dropdown; sidebar closed by default in embedded mode; bounded height; visible streaming indicator +- Completed final `chat-provider-fixes` follow-up round with three workstreams: + - `shared-trip-tool-access`: `get_trip_details` and `add_to_itinerary` now authorize `shared_with` members using `Q(user=user) | Q(shared_with=user)).distinct()`; `list_trips` remains owner-only + - `chat-regression-tests`: focused backend regression tests in `backend/server/chat/tests.py` for shared-trip access and required-param regex boundaries (9 tests, all pass) + - `chat-a11y-and-dropdown-polish`: aria-labels in `AITravelChat.svelte` now use i18n keys; settings dropdown closes on outside click and Escape; locale key parity across all 20 files - All three workstreams passed reviewer + tester validation -- Prior session completed `chat-loop-hardening`, `default-ai-settings`, `suggestion-add-flow` — all reviewed and tested +- Prior sessions completed: `chat-loop-hardening`, `default-ai-settings`, `suggestion-add-flow`, `chat-tool-grounding-and-confirmation`, `chat-tool-output-cleanup`, `embedded-chat-ux-polish` — all reviewed and tested ## Active Work - `chat-provider-fixes` plan complete — all workstreams implemented, reviewed, tested, documented @@ -19,6 +19,6 @@ - No automated test coverage for `DaySuggestionsView.post()` - No Playwright e2e test for tool summary reconstruction on conversation reload - LLM-generated name/location fields not truncated to `max_length=200` before `LocationSerializer` (low risk) -- `aria-label` values in `AITravelChat.svelte` sidebar toggle and settings button are hardcoded English (should use `$t()`) -- `
` settings dropdown in embedded chat does not auto-close on outside click -- `get_trip_details` excludes `shared_with` members from `filter(user=user)` — shared users get UUID context but tool returns DoesNotExist (pre-existing, low severity) +- Non-English locale `chat_a11y` values are English placeholders — requires human translation (separate concern) +- `outsideEvents` array includes both `pointerdown` and `mousedown` — double-fires but idempotent; could simplify to `['pointerdown', 'touchstart']` +- Escape handler in settings dropdown lacks `settingsOpen` guard — idempotent no-op, no functional consequence diff --git a/backend/server/chat/agent_tools.py b/backend/server/chat/agent_tools.py index c6ebdee6..964e5bf8 100644 --- a/backend/server/chat/agent_tools.py +++ b/backend/server/chat/agent_tools.py @@ -6,6 +6,7 @@ from datetime import date as date_cls import requests from django.contrib.contenttypes.models import ContentType from django.db import models +from django.db.models import Q from adventures.models import Collection, CollectionItineraryItem, Location @@ -322,7 +323,8 @@ def get_trip_details(user, collection_id: str | None = None): return {"error": "collection_id is required"} collection = ( - Collection.objects.filter(user=user) + Collection.objects.filter(Q(user=user) | Q(shared_with=user)) + .distinct() .prefetch_related( "locations", "transportation_set", @@ -460,7 +462,11 @@ def add_to_itinerary( "error": "collection_id, name, latitude, and longitude are required" } - collection = Collection.objects.get(id=collection_id, user=user) + collection = ( + Collection.objects.filter(Q(user=user) | Q(shared_with=user)) + .distinct() + .get(id=collection_id) + ) location = Location.objects.create( user=user, diff --git a/backend/server/chat/tests.py b/backend/server/chat/tests.py new file mode 100644 index 00000000..e4b42c8b --- /dev/null +++ b/backend/server/chat/tests.py @@ -0,0 +1,148 @@ +import json +from unittest.mock import patch + +from django.contrib.auth import get_user_model +from django.test import TestCase + +from adventures.models import Collection, CollectionItineraryItem +from chat.agent_tools import add_to_itinerary, get_trip_details +from chat.views import ChatViewSet + + +User = get_user_model() + + +class ChatAgentToolSharedTripAccessTests(TestCase): + def setUp(self): + self.owner = User.objects.create_user( + username="chat-owner", + email="chat-owner@example.com", + password="password123", + ) + self.shared_user = User.objects.create_user( + username="chat-shared", + email="chat-shared@example.com", + password="password123", + ) + self.non_member = User.objects.create_user( + username="chat-non-member", + email="chat-non-member@example.com", + password="password123", + ) + self.collection = Collection.objects.create( + user=self.owner, + name="Shared Trip", + ) + self.collection.shared_with.add(self.shared_user) + + def test_get_trip_details_allows_owner_access(self): + result = get_trip_details(self.owner, collection_id=str(self.collection.id)) + + self.assertIn("trip", result) + self.assertEqual(result["trip"]["id"], str(self.collection.id)) + self.assertEqual(result["trip"]["name"], self.collection.name) + + def test_get_trip_details_allows_shared_user_access(self): + result = get_trip_details( + self.shared_user, collection_id=str(self.collection.id) + ) + + self.assertIn("trip", result) + self.assertEqual(result["trip"]["id"], str(self.collection.id)) + + def test_get_trip_details_owner_also_in_shared_with_avoids_duplicates(self): + self.collection.shared_with.add(self.owner) + + result = get_trip_details(self.owner, collection_id=str(self.collection.id)) + + self.assertIn("trip", result) + self.assertEqual(result["trip"]["id"], str(self.collection.id)) + + @patch("adventures.models.background_geocode_and_assign") + def test_add_to_itinerary_allows_shared_user_access(self, _mock_background_geocode): + result = add_to_itinerary( + self.shared_user, + collection_id=str(self.collection.id), + name="Eiffel Tower", + latitude=48.85837, + longitude=2.294481, + ) + + self.assertTrue(result.get("success")) + self.assertEqual(result["location"]["name"], "Eiffel Tower") + self.assertTrue( + CollectionItineraryItem.objects.filter( + id=result["itinerary_item"]["id"], + collection=self.collection, + ).exists() + ) + + @patch("adventures.models.background_geocode_and_assign") + def test_add_to_itinerary_owner_also_in_shared_with_avoids_duplicates( + self, + _mock_background_geocode, + ): + self.collection.shared_with.add(self.owner) + + result = add_to_itinerary( + self.owner, + collection_id=str(self.collection.id), + name="Louvre Museum", + latitude=48.860611, + longitude=2.337644, + ) + + self.assertTrue(result.get("success")) + self.assertEqual(result["location"]["name"], "Louvre Museum") + + @patch("adventures.models.background_geocode_and_assign") + def test_non_member_access_remains_denied(self, _mock_background_geocode): + trip_result = get_trip_details( + self.non_member, + collection_id=str(self.collection.id), + ) + itinerary_result = add_to_itinerary( + self.non_member, + collection_id=str(self.collection.id), + name="Should Fail", + latitude=48.85837, + longitude=2.294481, + ) + + self.assertEqual( + trip_result, + { + "error": "collection_id is required and must reference a trip you can access" + }, + ) + self.assertEqual(itinerary_result, {"error": "Trip not found"}) + + +class ChatViewSetToolValidationBoundaryTests(TestCase): + def test_dates_is_required_matches_required_param_short_circuit(self): + self.assertTrue( + ChatViewSet._is_required_param_tool_error({"error": "dates is required"}) + ) + + def test_dates_non_empty_list_error_does_not_match_required_param_short_circuit( + self, + ): + self.assertFalse( + ChatViewSet._is_required_param_tool_error( + {"error": "dates must be a non-empty list"} + ) + ) + + def test_collection_access_error_does_not_short_circuit_required_param_regex(self): + error_text = ( + "collection_id is required and must reference a trip you can access" + ) + + self.assertFalse( + ChatViewSet._is_required_param_tool_error({"error": error_text}) + ) + self.assertFalse( + ChatViewSet._is_required_param_tool_error_message_content( + json.dumps({"error": error_text}) + ) + ) diff --git a/frontend/src/lib/components/AITravelChat.svelte b/frontend/src/lib/components/AITravelChat.svelte index 38d17ed9..9a2c2c9f 100644 --- a/frontend/src/lib/components/AITravelChat.svelte +++ b/frontend/src/lib/components/AITravelChat.svelte @@ -78,6 +78,7 @@ let selectedPlaceToAdd: PlaceResult | null = null; let selectedDate = ''; let settingsOpen = false; + let settingsDropdownRef: HTMLDetailsElement; const dispatch = createEventDispatcher<{ close: void; @@ -87,10 +88,44 @@ const MODEL_PREFS_STORAGE_KEY = 'voyage_chat_model_prefs'; $: promptTripContext = collectionName || destination || ''; - onMount(async () => { + onMount(() => { + void initializeChat(); + + const handleOutsideSettings = (event: Event) => { + if (!settingsOpen || !settingsDropdownRef) { + return; + } + + const target = event.target as Node | null; + if (target && !settingsDropdownRef.contains(target)) { + settingsOpen = false; + } + }; + + const handleSettingsEscape = (event: KeyboardEvent) => { + if (event.key === 'Escape') { + settingsOpen = false; + } + }; + + const outsideEvents: Array = ['pointerdown', 'mousedown', 'touchstart']; + outsideEvents.forEach((eventName) => { + document.addEventListener(eventName, handleOutsideSettings); + }); + document.addEventListener('keydown', handleSettingsEscape); + + return () => { + outsideEvents.forEach((eventName) => { + document.removeEventListener(eventName, handleOutsideSettings); + }); + document.removeEventListener('keydown', handleSettingsEscape); + }; + }); + + async function initializeChat(): Promise { await Promise.all([loadConversations(), loadProviderCatalog(), loadUserAISettings()]); await applyInitialDefaults(); - }); + } async function loadUserAISettings(): Promise { try { @@ -740,7 +775,9 @@ on:click={() => (sidebarOpen = !sidebarOpen)} aria-controls="chat-conversations-sidebar" aria-expanded={sidebarOpen} - aria-label={sidebarOpen ? 'Hide conversations' : 'Show conversations'} + aria-label={sidebarOpen + ? $t('chat_a11y.hide_conversations_aria') + : $t('chat_a11y.show_conversations_aria')} > {#if sidebarOpen}
-
+
⚙️ diff --git a/frontend/src/locales/ar.json b/frontend/src/locales/ar.json index 29a1510f..1b15972a 100644 --- a/frontend/src/locales/ar.json +++ b/frontend/src/locales/ar.json @@ -1,4 +1,9 @@ { + "chat_a11y": { + "show_conversations_aria": "Show conversations", + "hide_conversations_aria": "Hide conversations", + "ai_settings_aria": "AI settings" + }, "about": { "about": "عن", "attributions": "الصفات", diff --git a/frontend/src/locales/de.json b/frontend/src/locales/de.json index 042fbc51..0f9b2df1 100644 --- a/frontend/src/locales/de.json +++ b/frontend/src/locales/de.json @@ -1,4 +1,9 @@ { + "chat_a11y": { + "show_conversations_aria": "Show conversations", + "hide_conversations_aria": "Hide conversations", + "ai_settings_aria": "AI settings" + }, "about": { "about": "Über", "close": "Schließen", diff --git a/frontend/src/locales/en.json b/frontend/src/locales/en.json index 09989ad1..2487d97b 100644 --- a/frontend/src/locales/en.json +++ b/frontend/src/locales/en.json @@ -80,6 +80,11 @@ "why_fits": "Why it's a great fit", "error": "Failed to get suggestions. Please try again." }, + "chat_a11y": { + "show_conversations_aria": "Show conversations", + "hide_conversations_aria": "Hide conversations", + "ai_settings_aria": "AI settings" + }, "about": { "about": "About", "license": "Licensed under the GPL-3.0 License.", diff --git a/frontend/src/locales/es.json b/frontend/src/locales/es.json index c0cf0417..7d3eb704 100644 --- a/frontend/src/locales/es.json +++ b/frontend/src/locales/es.json @@ -31,6 +31,11 @@ "navigation": "Navegación", "worldtravel": "Viajes por el mundo" }, + "chat_a11y": { + "show_conversations_aria": "Show conversations", + "hide_conversations_aria": "Hide conversations", + "ai_settings_aria": "AI settings" + }, "about": { "about": "Acerca de", "license": "Licenciado bajo la Licencia GPL-3.0.", diff --git a/frontend/src/locales/fr.json b/frontend/src/locales/fr.json index 284c186d..ebe35a81 100644 --- a/frontend/src/locales/fr.json +++ b/frontend/src/locales/fr.json @@ -1,4 +1,9 @@ { + "chat_a11y": { + "show_conversations_aria": "Show conversations", + "hide_conversations_aria": "Hide conversations", + "ai_settings_aria": "AI settings" + }, "about": { "about": "À propos", "close": "Fermer", diff --git a/frontend/src/locales/hu.json b/frontend/src/locales/hu.json index bb6995dc..3d94c2a4 100644 --- a/frontend/src/locales/hu.json +++ b/frontend/src/locales/hu.json @@ -31,6 +31,11 @@ "navigation": "Navigáció", "worldtravel": "Világutazás" }, + "chat_a11y": { + "show_conversations_aria": "Show conversations", + "hide_conversations_aria": "Hide conversations", + "ai_settings_aria": "AI settings" + }, "about": { "about": "Névjegy", "license": "GPL-3.0 licenc alatt terjesztve.", diff --git a/frontend/src/locales/it.json b/frontend/src/locales/it.json index 7f389256..9ed41b37 100644 --- a/frontend/src/locales/it.json +++ b/frontend/src/locales/it.json @@ -1,4 +1,9 @@ { + "chat_a11y": { + "show_conversations_aria": "Show conversations", + "hide_conversations_aria": "Hide conversations", + "ai_settings_aria": "AI settings" + }, "about": { "about": "Di", "close": "Chiudi", diff --git a/frontend/src/locales/ja.json b/frontend/src/locales/ja.json index 1a2652b0..68ff8ac4 100644 --- a/frontend/src/locales/ja.json +++ b/frontend/src/locales/ja.json @@ -1,4 +1,9 @@ { + "chat_a11y": { + "show_conversations_aria": "Show conversations", + "hide_conversations_aria": "Hide conversations", + "ai_settings_aria": "AI settings" + }, "about": { "about": "について", "attributions": "帰属", diff --git a/frontend/src/locales/ko.json b/frontend/src/locales/ko.json index 25da11dd..a2549a6d 100644 --- a/frontend/src/locales/ko.json +++ b/frontend/src/locales/ko.json @@ -1,4 +1,9 @@ { + "chat_a11y": { + "show_conversations_aria": "Show conversations", + "hide_conversations_aria": "Hide conversations", + "ai_settings_aria": "AI settings" + }, "about": { "about": "소개", "close": "닫기", diff --git a/frontend/src/locales/nl.json b/frontend/src/locales/nl.json index 0c3169b8..b26d43e7 100644 --- a/frontend/src/locales/nl.json +++ b/frontend/src/locales/nl.json @@ -1,4 +1,9 @@ { + "chat_a11y": { + "show_conversations_aria": "Show conversations", + "hide_conversations_aria": "Hide conversations", + "ai_settings_aria": "AI settings" + }, "about": { "about": "Over", "close": "Sluiten", diff --git a/frontend/src/locales/no.json b/frontend/src/locales/no.json index 7e2f8cee..f84daa49 100644 --- a/frontend/src/locales/no.json +++ b/frontend/src/locales/no.json @@ -31,6 +31,11 @@ "navigation": "Navigasjon", "worldtravel": "Verdensreise" }, + "chat_a11y": { + "show_conversations_aria": "Show conversations", + "hide_conversations_aria": "Hide conversations", + "ai_settings_aria": "AI settings" + }, "about": { "about": "Om", "license": "Lisensiert under GPL-3.0-lisensen.", diff --git a/frontend/src/locales/pl.json b/frontend/src/locales/pl.json index 7af96d9b..55b80809 100644 --- a/frontend/src/locales/pl.json +++ b/frontend/src/locales/pl.json @@ -31,6 +31,11 @@ "navigation": "Nawigacja", "worldtravel": "Światowa podróż" }, + "chat_a11y": { + "show_conversations_aria": "Show conversations", + "hide_conversations_aria": "Hide conversations", + "ai_settings_aria": "AI settings" + }, "about": { "about": "O aplikacji", "license": "Licencjonowane na licencji GPL-3.0.", diff --git a/frontend/src/locales/pt-br.json b/frontend/src/locales/pt-br.json index a32a7e02..4b4fbdb0 100644 --- a/frontend/src/locales/pt-br.json +++ b/frontend/src/locales/pt-br.json @@ -1,4 +1,9 @@ { + "chat_a11y": { + "show_conversations_aria": "Show conversations", + "hide_conversations_aria": "Hide conversations", + "ai_settings_aria": "AI settings" + }, "about": { "about": "Sobre", "attributions": "Atribuições", diff --git a/frontend/src/locales/ro.json b/frontend/src/locales/ro.json index eaf28ac4..3c0cf7e7 100644 --- a/frontend/src/locales/ro.json +++ b/frontend/src/locales/ro.json @@ -31,6 +31,11 @@ }, "navigation": "Navigație" }, + "chat_a11y": { + "show_conversations_aria": "Show conversations", + "hide_conversations_aria": "Hide conversations", + "ai_settings_aria": "AI settings" + }, "about": { "about": "Despre", "license": "Licențiat sub licența GPL-3.0.", diff --git a/frontend/src/locales/ru.json b/frontend/src/locales/ru.json index 7b2dff8f..66b7a5ec 100644 --- a/frontend/src/locales/ru.json +++ b/frontend/src/locales/ru.json @@ -31,6 +31,11 @@ "navigation": "Навигация", "worldtravel": "Путешествие по миру" }, + "chat_a11y": { + "show_conversations_aria": "Show conversations", + "hide_conversations_aria": "Hide conversations", + "ai_settings_aria": "AI settings" + }, "about": { "about": "О программе", "license": "Лицензировано под лицензией GPL-3.0.", diff --git a/frontend/src/locales/sk.json b/frontend/src/locales/sk.json index 7918ffef..08d5a2d6 100644 --- a/frontend/src/locales/sk.json +++ b/frontend/src/locales/sk.json @@ -31,6 +31,11 @@ "navigation": "Navigácia", "worldtravel": "Svetové cestovanie" }, + "chat_a11y": { + "show_conversations_aria": "Show conversations", + "hide_conversations_aria": "Hide conversations", + "ai_settings_aria": "AI settings" + }, "about": { "about": "O aplikácii", "license": "Licencované pod licenciou GPL-3.0.", diff --git a/frontend/src/locales/sv.json b/frontend/src/locales/sv.json index 98c93937..475847a4 100644 --- a/frontend/src/locales/sv.json +++ b/frontend/src/locales/sv.json @@ -1,4 +1,9 @@ { + "chat_a11y": { + "show_conversations_aria": "Show conversations", + "hide_conversations_aria": "Hide conversations", + "ai_settings_aria": "AI settings" + }, "about": { "about": "Om", "close": "Stäng", diff --git a/frontend/src/locales/tr.json b/frontend/src/locales/tr.json index cf643205..c59aad53 100644 --- a/frontend/src/locales/tr.json +++ b/frontend/src/locales/tr.json @@ -31,6 +31,11 @@ "navigation": "Navigasyon", "worldtravel": "Dünya Seyahati" }, + "chat_a11y": { + "show_conversations_aria": "Show conversations", + "hide_conversations_aria": "Hide conversations", + "ai_settings_aria": "AI settings" + }, "about": { "about": "Hakkında", "license": "GPL-3.0 Lisansı altında lisanslanmıştır.", diff --git a/frontend/src/locales/uk.json b/frontend/src/locales/uk.json index 12952788..784fe253 100644 --- a/frontend/src/locales/uk.json +++ b/frontend/src/locales/uk.json @@ -1,4 +1,9 @@ { + "chat_a11y": { + "show_conversations_aria": "Show conversations", + "hide_conversations_aria": "Hide conversations", + "ai_settings_aria": "AI settings" + }, "about": { "about": "про", "attributions": "Атрибуції", diff --git a/frontend/src/locales/zh.json b/frontend/src/locales/zh.json index 2af37d2c..3cfe79d7 100644 --- a/frontend/src/locales/zh.json +++ b/frontend/src/locales/zh.json @@ -31,6 +31,11 @@ "navigation": "导航", "worldtravel": "环球旅行" }, + "chat_a11y": { + "show_conversations_aria": "Show conversations", + "hide_conversations_aria": "Hide conversations", + "ai_settings_aria": "AI settings" + }, "about": { "about": "关于", "license": "根据 GPL-3.0 许可证授权。",