diff --git a/CHANGELOG.md b/CHANGELOG.md index c5c7917..1cecc32 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -643,6 +643,32 @@ ### Fixed +- **The phone drawer keeps Tab inside it,** as its `aria-modal` promised, and + hides the chat behind it from screen readers (`aria-hidden`; `inert` would + blur the button that opened it, and focus couldn't return there). The trap + is shared with the Sources sheet (`features/chat/focus.js`). +- **A question sent while scrolled up is shown,** and the view follows its + answer again. Scrolling up while it streams still stops the following. +- **A finished answer is announced to screen readers:** "Answer ready, N + sources" (one per cited document, as the seals count them), from a status + region that stays mounted. The streaming region was removed at `done`, so + nothing was said. History never announces. +- **A connection lost mid-answer says PolicyPal can't be reached,** not the + browser's raw error, and `apiStream` always cancels its reader, so the + connection is let go however the stream ends. An abort is still rethrown + as itself. +- ADR 0027 notes that its timeout bound left out the CMS pricing call: at + most 8 s in each of two tool rounds, so the theoretical bound passes + Azure's 240 s ingress timeout. +- Renaming a thread checks the 1 to 200 characters after trimming, as ADR + 0027 says. A title that fits once its spaces are removed was refused. +- **A citation keeps its seal number after a reload.** A streamed answer + listed its sources in retrieval order, and a reload in relevance order, so + the numbers could change. Both now list them most relevant first, ties by + chunk and label compared byte-wise (`COLLATE "C"` in SQL). +- The test of a streamed answer whose save fails to commit now runs the real + spend recording. It checks that the day's total is the spend once (not 0, + not doubled) and that no answer or citation is left behind. - **A streamed answer's spend survives a failed save.** Spend is marked as recorded only after its commit succeeds. Before, a rolled-back commit took the spend with it, and it was never recorded again. @@ -754,6 +780,9 @@ ### Security +- The database engine hides bound values (`hide_parameters=True`), so an + email, question or answer never appears in a SQL error or its log line. + Postgres's own message can still quote a value it failed to parse. - **A streamed round that never reports its usage is counted by estimate:** the whole prompt plus the output cap. This covers a client that disconnects, Stop, or a dropped connection. Without it, ending streams diff --git a/docs/decisions/0027-streaming-answers-and-cited-passages.md b/docs/decisions/0027-streaming-answers-and-cited-passages.md index c913617..8d5342f 100644 --- a/docs/decisions/0027-streaming-answers-and-cited-passages.md +++ b/docs/decisions/0027-streaming-answers-and-cited-passages.md @@ -15,6 +15,12 @@ It amends two earlier ADRs: Sub-project 3 of the California expansion, which had planned to take this number, takes ADR 0028 instead. +Corrected on 2026-09-27: the timeout bound under Consequences counts only +the model calls, which with retries alone reach 240 s. Each of the two tool +rounds can add a live CMS pricing call of up to 8 s +(`cms_live_timeout_seconds`), so the theoretical bound is past Azure's +ingress timeout, not close to it. The usual worst case is still well under it. + ## Context The chat worked, but three things made it read like a prototype: diff --git a/frontend/src/features/chat/AppShell.jsx b/frontend/src/features/chat/AppShell.jsx index 642a0ab..3edc0f9 100644 --- a/frontend/src/features/chat/AppShell.jsx +++ b/frontend/src/features/chat/AppShell.jsx @@ -1,13 +1,13 @@ import { AnimatePresence, motion } from "motion/react"; import { useEffect, useRef } from "react"; import { useMediaQuery } from "../../hooks/useMediaQuery"; +import { FOCUSABLE, trapTab } from "./focus"; import "./shell.css"; const SIDEBAR_WIDTH = 276; -const FOCUSABLE = 'a[href], button:not([disabled]), input, [tabindex]:not([tabindex="-1"])'; -// The phone's sidebar: a drawer over the chat, with focus moved into it and -// returned when it closes. +// The phone's sidebar: a drawer over the chat, with focus moved into it, kept +// there, and returned when it closes. function Drawer({ children, onClose }) { const ref = useRef(null); @@ -36,9 +36,12 @@ function Drawer({ children, onClose }) { aria-modal="true" aria-label="Your questions" onKeyDown={(event) => { - if (event.key !== "Escape") return; - event.stopPropagation(); // not also the page's Esc - onClose(); + if (event.key === "Escape") { + event.stopPropagation(); // not also the page's Esc + onClose(); + } else { + trapTab(event, ref.current); + } }} initial={{ x: "-100%" }} animate={{ x: 0 }} @@ -75,7 +78,12 @@ export default function AppShell({ sidebar, main, panel = null, sidebarHidden = {sidebar} )} - {main} + {/* Behind an open drawer the chat is hidden, as aria-modal says; the + drawer keeps Tab. Not inert: that would blur the button that opened + the drawer before the drawer could note it, and focus couldn't go back. */} +
+ {main} +
{panel} ); diff --git a/frontend/src/features/chat/AppShell.test.jsx b/frontend/src/features/chat/AppShell.test.jsx index 19c610e..eef0192 100644 --- a/frontend/src/features/chat/AppShell.test.jsx +++ b/frontend/src/features/chat/AppShell.test.jsx @@ -37,6 +37,7 @@ describe("AppShell", () => { expect(container.firstChild).toHaveClass("collapsed"); expect(screen.queryByRole("complementary", { name: "Your questions" })).not.toBeInTheDocument(); expect(container.querySelector(".side-wrap")).toHaveAttribute("inert"); + expect(container.querySelector(".shell-main")).not.toHaveAttribute("aria-hidden"); }); describe("on a phone", () => { @@ -66,6 +67,47 @@ describe("AppShell", () => { expect(onCloseDrawer).toHaveBeenCalledTimes(2); }); + it("keeps Tab inside the open drawer, and the chat behind it out of reach", async () => { + const twoButtons = ( + + ); + const chat = ( +
+ +
+ ); + const { container } = render(); + const drawer = screen.getByRole("dialog", { name: "Your questions" }); + + for (let i = 0; i < 4; i += 1) { + await userEvent.tab(); + expect(drawer).toContainElement(document.activeElement); + } + await userEvent.tab({ shift: true }); + expect(drawer).toContainElement(document.activeElement); + expect(container.querySelector(".shell-main")).toHaveAttribute("aria-hidden", "true"); + }); + + it("gives the chat back, and focus to what opened the drawer, once it closes", async () => { + const opener = ( +
+ +
+ ); + const { container, rerender } = render(); + screen.getByRole("button", { name: "Open your questions" }).focus(); + + rerender(); + expect(screen.getByRole("button", { name: "New question" })).toHaveFocus(); + rerender(); + + await waitFor(() => expect(screen.getByRole("button", { name: "Open your questions" })).toHaveFocus()); + expect(container.querySelector(".shell-main")).not.toHaveAttribute("aria-hidden"); + }); + it("removes the drawer once closed", async () => { const { rerender } = render(); rerender(); diff --git a/frontend/src/features/chat/focus.js b/frontend/src/features/chat/focus.js new file mode 100644 index 0000000..ec07384 --- /dev/null +++ b/frontend/src/features/chat/focus.js @@ -0,0 +1,19 @@ +// What Tab can reach in the chat's modal layers: the phone drawer and the +// Sources sheet. +export const FOCUSABLE = 'a[href], button:not([disabled]), input:not([disabled]), [tabindex]:not([tabindex="-1"])'; + +// Tab and Shift+Tab stay inside a modal layer, wrapping at either end. Moved +// by hand, since focus can sit on an element that isn't itself in the tab +// order (a selected source card). +export function trapTab(event, container) { + if (event.key !== "Tab") return; + const focusable = [...container.querySelectorAll(FOCUSABLE)]; + if (focusable.length === 0) return; + const current = document.activeElement; + const position = (el) => current.compareDocumentPosition(el); + const after = focusable.filter((el) => el !== current && position(el) & Node.DOCUMENT_POSITION_FOLLOWING); + const before = focusable.filter((el) => el !== current && position(el) & Node.DOCUMENT_POSITION_PRECEDING); + const next = event.shiftKey ? (before.at(-1) ?? focusable.at(-1)) : (after[0] ?? focusable[0]); + event.preventDefault(); + next.focus(); +} diff --git a/frontend/src/features/chat/focus.test.js b/frontend/src/features/chat/focus.test.js new file mode 100644 index 0000000..7fbf4c4 --- /dev/null +++ b/frontend/src/features/chat/focus.test.js @@ -0,0 +1,53 @@ +import { afterEach, describe, expect, it, vi } from "vitest"; +import { trapTab } from "./focus"; + +function layer(html) { + const container = document.createElement("div"); + container.innerHTML = html; + document.body.append(container); + return container; +} + +function tab(container, shiftKey = false) { + const event = { key: "Tab", shiftKey, preventDefault: vi.fn() }; + trapTab(event, container); + return event; +} + +afterEach(() => { + document.body.innerHTML = ""; +}); + +describe("trapTab", () => { + it("wraps from the last element to the first, and back with Shift", () => { + const container = layer('Three'); + const [one, , three] = container.querySelectorAll("button, input, a"); + + three.focus(); + expect(tab(container).preventDefault).toHaveBeenCalled(); + expect(one).toHaveFocus(); + + tab(container, true); + expect(three).toHaveFocus(); + }); + + it("moves on from an element outside the tab order, such as a selected card", () => { + const container = layer('
Card
'); + container.querySelector("div").focus(); + + tab(container); + expect(container.querySelectorAll("button")[1]).toHaveFocus(); + }); + + it("skips disabled controls and leaves other keys alone", () => { + const container = layer(""); + const [one, , two] = container.querySelectorAll("button"); + one.focus(); + + tab(container); + expect(two).toHaveFocus(); + const other = { key: "Enter", preventDefault: vi.fn() }; + trapTab(other, container); + expect(other.preventDefault).not.toHaveBeenCalled(); + }); +}); diff --git a/frontend/src/features/chat/shell.css b/frontend/src/features/chat/shell.css index 6d00bcc..d8b34a6 100644 --- a/frontend/src/features/chat/shell.css +++ b/frontend/src/features/chat/shell.css @@ -19,6 +19,12 @@ grid-template-columns: minmax(0, 1fr); } +/* Only there to hide the main column behind the phone drawer: its child + stays the grid item. */ +.shell-main { + display: contents; +} + .shell-error { display: flex; flex-direction: column; diff --git a/frontend/src/features/chat/sources/SourcesPanel.jsx b/frontend/src/features/chat/sources/SourcesPanel.jsx index 77db55b..1d9d406 100644 --- a/frontend/src/features/chat/sources/SourcesPanel.jsx +++ b/frontend/src/features/chat/sources/SourcesPanel.jsx @@ -4,26 +4,10 @@ import { useEffect, useRef } from "react"; import IconButton from "../../../components/IconButton"; import { useMediaQuery } from "../../../hooks/useMediaQuery"; import { sourceNumbers } from "../../../utils/markers"; +import { trapTab } from "../focus"; import SourceCard from "./SourceCard"; import "./sources.css"; -const FOCUSABLE = 'a[href], button:not([disabled]), [tabindex]:not([tabindex="-1"])'; - -// Tab and Shift+Tab stay inside a modal sheet, wrapping at either end. Moved -// by hand, since focus can sit on a card that isn't itself in the tab order. -function trapTab(event, container) { - if (event.key !== "Tab") return; - const focusable = [...container.querySelectorAll(FOCUSABLE)]; - if (focusable.length === 0) return; - const current = document.activeElement; - const position = (el) => current.compareDocumentPosition(el); - const after = focusable.filter((el) => el !== current && position(el) & Node.DOCUMENT_POSITION_FOLLOWING); - const before = focusable.filter((el) => el !== current && position(el) & Node.DOCUMENT_POSITION_PRECEDING); - const next = event.shiftKey ? (before.at(-1) ?? focusable.at(-1)) : (after[0] ?? focusable[0]); - event.preventDefault(); - next.focus(); -} - // The cited passages of one answer (Main.dc.html's panel): a column beside the // transcript, or a bottom sheet with a scrim at 720 px and below (Mobile.dc.html). export default function SourcesPanel({ message, selectedSourceId, hoveredSourceId, onSelect, onClose }) { diff --git a/frontend/src/features/chat/transcript/Transcript.jsx b/frontend/src/features/chat/transcript/Transcript.jsx index a38b296..1416f65 100644 --- a/frontend/src/features/chat/transcript/Transcript.jsx +++ b/frontend/src/features/chat/transcript/Transcript.jsx @@ -1,6 +1,7 @@ import { useEffect, useRef, useState } from "react"; import ErrorBanner from "../../../components/ErrorBanner"; import { toExchanges } from "../../../utils/exchanges"; +import { sourceNumbers } from "../../../utils/markers"; import EmptyState from "./EmptyState"; import Exchange from "./Exchange"; import JumpToLatest from "./JumpToLatest"; @@ -11,6 +12,17 @@ const PINNED_PX = 80; // Further than this above it, "Jump to latest" appears (spec ยง4.3). const JUMP_PX = 200; +// What a screen reader hears when an answer finishes: the streaming region is +// replaced by the saved answer, which announces nothing. Sources are counted +// as the seals number them, one per document. +function readyText(messages, finishedId) { + const answer = finishedId && messages.find((m) => m.id === finishedId); + if (!answer) return ""; + const count = sourceNumbers(answer.sources).size; + if (count === 0) return "Answer ready"; + return `Answer ready, ${count} ${count === 1 ? "source" : "sources"}`; +} + function Content({ messages, status, error, onPrompt, onRetry, children }) { if (status === "loading" && messages.length === 0) { return ( @@ -71,10 +83,15 @@ export default function Transcript({ endRef.current?.scrollIntoView({ block: "end", behavior: still ? "auto" : "smooth" }); } - // The scroll position is the DOM's: follow new text only while pinned. + // The scroll position is the DOM's: follow new text only while pinned. A + // question just sent (a draft appearing) is always shown, and pins the view + // again, wherever the reader had scrolled to. + const drafting = useRef(false); useEffect(() => { + if (live && !drafting.current) pinned.current = true; + drafting.current = Boolean(live); if (pinned.current) endRef.current?.scrollIntoView({ block: "end" }); - }, [messages.length, live?.text, live?.stages.length, live?.notices.length]); + }, [messages.length, live]); return ( <> @@ -103,6 +120,11 @@ export default function Transcript({
{away && } + {/* Always mounted, and empty while an answer streams, so the next + answer is announced even when its text is the same as the last. */} +

+ {live ? "" : readyText(messages, finishedId)} +

); } diff --git a/frontend/src/features/chat/transcript/Transcript.test.jsx b/frontend/src/features/chat/transcript/Transcript.test.jsx index c6db4e1..524fff2 100644 --- a/frontend/src/features/chat/transcript/Transcript.test.jsx +++ b/frontend/src/features/chat/transcript/Transcript.test.jsx @@ -129,4 +129,51 @@ describe("Transcript", () => { rerender(); expect(scrollIntoView).toHaveBeenCalledTimes(1); }); + + it("announces a finished answer with its count of cited documents", () => { + const sources = [ + { id: "s1", source: "Plan A - Summary of Benefits.pdf", chunk_id: "c1", relevance: 0.9 }, + { id: "s2", source: "Plan A - Summary of Benefits.pdf", chunk_id: "c2", relevance: 0.8 }, + { id: "s3", source: "wiki_Copayment.txt", chunk_id: "c3", relevance: 0.7 }, + ]; + const cited = [messages[0], { ...messages[1], sources }]; + const { rerender } = renderTranscript({ messages: cited }); + const status = () => screen.getByRole("status"); + expect(status()).toBeEmptyDOMElement(); // history never announces + + rerender(); + expect(status()).toHaveTextContent("Answer ready, 2 sources"); + }); + + it("says just that the answer is ready when it cites nothing, and stays quiet while one streams", () => { + const live = { questionId: "q2", stages: ["writing"], notices: [], text: "It" }; + const { rerender } = renderTranscript({ finishedId: "a1" }); + const status = () => screen.getByRole("status"); + expect(status()).toHaveTextContent(/^Answer ready$/); + + const asked = [...messages, { id: "q2", role: "user", content: "And a copay?", created_at: "2026-01-01T00:01:00Z" }]; + rerender(); + expect(status()).toBeEmptyDOMElement(); + }); + + it("shows a question you send even while scrolled up, then follows its answer", () => { + const scrollIntoView = vi.fn(); + const props = { messages, status: "ready", live: null, onOpenSource: vi.fn() }; + const { container, rerender } = render(); + container.querySelector(".transcript-end").scrollIntoView = scrollIntoView; + const scroller = container.querySelector(".scroll"); + scrollTo(scroller, { top: 100 }); + + const question = { id: "temp-1", role: "user", content: "And a copay?", created_at: "2026-01-01T00:01:00Z" }; + const live = { questionId: "temp-1", stages: [], notices: [], text: "" }; + rerender(); + expect(scrollIntoView).toHaveBeenCalledTimes(1); + + rerender(); + expect(scrollIntoView).toHaveBeenCalledTimes(2); + + scrollTo(scroller, { top: 100 }); // scrolling up again still stops the following + rerender(); + expect(scrollIntoView).toHaveBeenCalledTimes(2); + }); }); diff --git a/frontend/src/pages/ChatPage.test.jsx b/frontend/src/pages/ChatPage.test.jsx index e7e72e4..8ea4ff0 100644 --- a/frontend/src/pages/ChatPage.test.jsx +++ b/frontend/src/pages/ChatPage.test.jsx @@ -242,7 +242,8 @@ describe("ChatPage", () => { await user.type(screen.getByRole("textbox", { name: "Thread name" }), " 101{Enter}"); expect(renameThread).toHaveBeenCalledWith("t1", "Deductibles 101"); - expect(await screen.findByRole("status")).toHaveTextContent("Couldn't rename this thread. Try again."); + // By its text: the transcript keeps a status region of its own. + expect(await screen.findByText("Couldn't rename this thread. Try again.")).toHaveAttribute("role", "status"); }); it("renames the open thread from its title", async () => { diff --git a/frontend/src/services/apiClient.js b/frontend/src/services/apiClient.js index 50487bb..f1a341a 100644 --- a/frontend/src/services/apiClient.js +++ b/frontend/src/services/apiClient.js @@ -68,7 +68,8 @@ export async function apiFetch(path, { method = "GET", token, body } = {}) { } // A POST answered with text/event-stream. Errors before the stream reject as -// apiFetch's do; an abort is rethrown as the AbortError it is. +// apiFetch's do, and so does a connection lost part-way; an abort is rethrown +// as the AbortError it is. export async function apiStream(path, { token, body, signal, onEvent }) { const headers = { "Content-Type": "application/json", Accept: "text/event-stream" }; if (token) headers.Authorization = `Bearer ${token}`; @@ -90,10 +91,19 @@ export async function apiStream(path, { token, body, signal, onEvent }) { const parser = createSseParser(onEvent); const reader = response.body.getReader(); const decoder = new TextDecoder(); - for (;;) { - const { value, done } = await reader.read(); - if (done) break; - parser.push(decoder.decode(value, { stream: true })); + try { + for (;;) { + const { value, done } = await reader.read(); + if (done) break; + parser.push(decoder.decode(value, { stream: true })); + } + } catch (err) { + if (err?.name === "AbortError") throw err; + throw new ApiError(UNREACHABLE, 0); + } finally { + // Frees the connection whichever way the loop ended; a finished stream + // makes this a no-op, and a failed one can't fail it any further. + reader.cancel().catch(() => {}); } parser.push(decoder.decode()); parser.end(); diff --git a/frontend/src/services/apiClient.test.js b/frontend/src/services/apiClient.test.js index c0700e6..95e2ef6 100644 --- a/frontend/src/services/apiClient.test.js +++ b/frontend/src/services/apiClient.test.js @@ -108,6 +108,17 @@ function streamOf(...chunks) { }); } +// A body whose connection drops after the first chunk, with `failure` as the +// browser's rejection, and a cancel to watch. +function droppingBody(chunk, failure) { + const read = vi + .fn() + .mockResolvedValueOnce({ value: new TextEncoder().encode(chunk), done: false }) + .mockRejectedValueOnce(failure); + const cancel = vi.fn().mockResolvedValue(undefined); + return { getReader: () => ({ read, cancel }), cancel }; +} + describe("apiStream", () => { afterEach(() => { vi.unstubAllGlobals(); @@ -155,6 +166,30 @@ describe("apiStream", () => { expect(error.name).toBe("AbortError"); }); + it("says PolicyPal can't be reached when the connection drops mid-stream, and lets the reader go", async () => { + const body = droppingBody('event: delta\ndata: {"text":"a"}\n\n', new TypeError("network error")); + vi.stubGlobal("fetch", vi.fn().mockResolvedValue({ ok: true, status: 200, body })); + const onEvent = vi.fn(); + + const error = await apiStream("/x", { token: "tok", body: {}, onEvent }).catch((e) => e); + + expect(error).toBeInstanceOf(ApiError); + expect(error.message).toMatch(/Can't reach PolicyPal/); + expect(error.message).not.toMatch(/network error/); + expect(onEvent).toHaveBeenCalledWith({ event: "delta", data: { text: "a" } }); + expect(body.cancel).toHaveBeenCalledOnce(); + }); + + it("rethrows an abort mid-stream as the AbortError it is, and lets the reader go", async () => { + const body = droppingBody('event: stage\ndata: {"stage":"searching"}\n\n', new DOMException("aborted", "AbortError")); + vi.stubGlobal("fetch", vi.fn().mockResolvedValue({ ok: true, status: 200, body })); + + const error = await apiStream("/x", { token: "tok", body: {}, onEvent: vi.fn() }).catch((e) => e); + + expect(error.name).toBe("AbortError"); + expect(body.cancel).toHaveBeenCalledOnce(); + }); + it("says PolicyPal can't be reached when the network fails", async () => { vi.stubGlobal("fetch", vi.fn().mockRejectedValue(new TypeError("Failed to fetch"))); await expect(apiStream("/x", { token: "tok", body: {}, onEvent: vi.fn() })).rejects.toThrow( diff --git a/src/api/routes/chat.py b/src/api/routes/chat.py index c67107d..1c0122d 100644 --- a/src/api/routes/chat.py +++ b/src/api/routes/chat.py @@ -138,10 +138,12 @@ def _save_answer(db, thread, question: str, result: Answer) -> Message: thread_id=thread.id, role="assistant", content=result.text, + # In the order a reload returns them (Message.sources), so a seal keeps + # its number: the live answer is serialized from this list as built. sources=[ MessageSource(chunk_id=c.chunk_id, source=c.source, relevance=c.score, content_sha256=content_hash(c.content)) - for c in result.chunks + for c in sorted(result.chunks, key=lambda c: (-c.score, c.chunk_id, c.source)) ], # A snapshot of what the answer showed, in the order shown (ADR 0011). plans=[_plan_row(position, plan) for position, plan in enumerate(result.plans)], diff --git a/src/core/db.py b/src/core/db.py index 89bcc30..cc72051 100644 --- a/src/core/db.py +++ b/src/core/db.py @@ -6,7 +6,9 @@ from ..policypal.config import settings -engine = create_engine(settings.database_url, pool_pre_ping=True) +# hide_parameters: bound values (emails, questions, answers) stay out of SQL +# errors and logs; the statement itself is still shown. +engine = create_engine(settings.database_url, pool_pre_ping=True, hide_parameters=True) SessionLocal = sessionmaker(bind=engine, autoflush=False, expire_on_commit=False) diff --git a/src/models/chat.py b/src/models/chat.py index 1c5aac5..57602cd 100644 --- a/src/models/chat.py +++ b/src/models/chat.py @@ -63,8 +63,15 @@ class Message(Base): created_at: Mapped[datetime] = mapped_column(DateTime(timezone=True), server_default=func.now(), nullable=False) thread: Mapped["Thread"] = relationship(back_populates="messages") + # Most relevant first; (chunk_id, source) is unique per answer, so ties are + # ordered too, byte-wise ("C") as Python compares them: _save_answer builds + # the list in this same order. sources: Mapped[list["MessageSource"]] = relationship( - back_populates="message", cascade="all, delete-orphan", order_by="MessageSource.relevance.desc()" + back_populates="message", + cascade="all, delete-orphan", + order_by=lambda: ( + MessageSource.relevance.desc(), MessageSource.chunk_id.collate("C"), MessageSource.source.collate("C") + ), ) plans: Mapped[list["MessagePlan"]] = relationship( back_populates="message", cascade="all, delete-orphan", order_by="MessagePlan.position" diff --git a/src/schemas/chat.py b/src/schemas/chat.py index 2a08ced..172ae14 100644 --- a/src/schemas/chat.py +++ b/src/schemas/chat.py @@ -10,17 +10,14 @@ class ThreadCreateRequest(BaseModel): class ThreadRenameRequest(BaseModel): - # max_length is checked before the validator strips, so a title padded - # past 200 characters is refused too. - title: str = Field(max_length=200) + # 1 to 200 characters after trimming (ADR 0027): stripped before the + # length is checked, and anything but a string is left for `str` to refuse. + title: str = Field(min_length=1, max_length=200) - @field_validator("title") + @field_validator("title", mode="before") @classmethod - def _not_blank(cls, value: str) -> str: - value = value.strip() - if not value: - raise ValueError("title must not be blank") - return value + def _trimmed(cls, value): + return value.strip() if isinstance(value, str) else value class ThreadResponse(BaseModel): diff --git a/tests/test_api_logging.py b/tests/test_api_logging.py index 30ee799..b8836d1 100644 --- a/tests/test_api_logging.py +++ b/tests/test_api_logging.py @@ -9,8 +9,11 @@ from logging.handlers import RotatingFileHandler import pytest +from sqlalchemy import text +from sqlalchemy.exc import DBAPIError from src.core import logging as app_logging +from src.core.db import engine def test_creating_the_app_installs_the_file_handler(app): @@ -30,6 +33,18 @@ def test_the_libraries_that_would_log_secrets_stay_at_warning(app): assert logging.getLogger(name).level == logging.WARNING +def test_a_database_error_leaves_out_the_values_it_was_given(): + """SQLAlchemy's error text, which is logged, carries the bound values by + default: an email, a question or an answer. (Postgres's own message can + still quote a value it failed on; this error doesn't, so only SQLAlchemy's + part is tested.)""" + with engine.connect() as connection, pytest.raises(DBAPIError) as caught: + connection.execute(text("SELECT :value, 1 / 0"), {"value": "someone@example.com"}) + + assert "someone@example.com" not in str(caught.value) + assert "hidden due to hide_parameters" in str(caught.value) + + @pytest.fixture def production(monkeypatch): monkeypatch.setattr(app_logging.settings, "environment", "production") diff --git a/tests/test_chat.py b/tests/test_chat.py index b99f378..ccdbc07 100644 --- a/tests/test_chat.py +++ b/tests/test_chat.py @@ -409,9 +409,9 @@ def test_rename_does_not_move_a_thread_up_the_list(client): assert datetime.fromisoformat(after[1]["updated_at"]) == week_ago -@pytest.mark.parametrize("title", ["", " ", "x" * 201]) -def test_rename_rejects_a_blank_or_long_title(client, title): - headers = _auth_headers(client, email=f"rename-bad{len(title)}@example.com") +@pytest.mark.parametrize("title", ["", " ", "x" * 201, " " + "x" * 201 + " ", 123, None]) +def test_rename_rejects_a_blank_long_or_non_text_title(client, title): + headers = _auth_headers(client, email=f"rename-bad-{uuid.uuid4().hex[:8]}@example.com") thread_id = _new_thread(client, headers) resp = client.patch(f"/api/chat/threads/{thread_id}", json={"title": title}, headers=headers) @@ -419,13 +419,15 @@ def test_rename_rejects_a_blank_or_long_title(client, title): assert resp.status_code == 422 -def test_rename_accepts_a_title_of_exactly_200_characters(client): - headers = _auth_headers(client, email="rename-200@example.com") +@pytest.mark.parametrize("title", ["x" * 200, " " + "x" * 200 + " "]) +def test_rename_accepts_200_characters_counted_after_trimming(client, title): + headers = _auth_headers(client, email=f"rename-200-{len(title)}@example.com") thread_id = _new_thread(client, headers) - resp = client.patch(f"/api/chat/threads/{thread_id}", json={"title": "x" * 200}, headers=headers) + resp = client.patch(f"/api/chat/threads/{thread_id}", json={"title": title}, headers=headers) assert resp.status_code == 200 + assert resp.get_json()["title"] == "x" * 200 @pytest.mark.parametrize("thread_id", ["00000000-0000-0000-0000-000000000000", "not-a-uuid"]) diff --git a/tests/test_chat_stream.py b/tests/test_chat_stream.py index 195da7e..3098cde 100644 --- a/tests/test_chat_stream.py +++ b/tests/test_chat_stream.py @@ -6,13 +6,17 @@ import flask import openai import pytest +from sqlalchemy import func, select from sqlalchemy.orm import Session from src.api import deps -from src.models.chat import MessageSource +from src.models.chat import Message, MessageSource +from src.models.user import User from src.services.generation import Answer, Delta, Done, Notice, Reset, Stage from src.services.passages import content_hash from src.services.retrieval import RetrievedChunk +from src.services.usage import record_tokens as real_record +from src.services.usage import tokens_used_today from tests.test_chat import _auth_headers CHUNKS = [RetrievedChunk("c1", "A deductible is...", "wiki_Health.txt", 0.9)] @@ -258,24 +262,73 @@ def source(*_args, **_kwargs): def test_spend_is_recorded_again_when_saving_the_answer_fails_to_commit(client): """The commit that saves the answer also carries the spend. If it fails, - the rollback undoes both, and the spend must still be recorded.""" + the rollback undoes both: the spend is recorded again, once, and nothing + of the answer is left behind.""" headers, thread_id = _thread(client, "stream-commit@example.com") real_commit = Session.commit - state = {"recorded": 0, "failed": False} + state = {"spends": [], "failed": False} - def record(*_args): - state["recorded"] += 1 + def record(db, user_id, tokens): + state["spends"].append(tokens) + real_record(db, user_id, tokens) def flaky_commit(session): - if state["recorded"] == 1 and not state["failed"]: + if len(state["spends"]) == 1 and not state["failed"]: state["failed"] = True raise RuntimeError("commit failed") return real_commit(session) with patch("src.api.routes.chat.answer_query_events", return_value=iter([Done(Answer("A.", CHUNKS))])), \ + patch("src.api.routes.chat.token_usage", return_value=21), \ patch("src.api.routes.chat.record_tokens", side_effect=record), \ patch.object(Session, "commit", flaky_commit): resp = client.post(_url(thread_id), json={"content": "x"}, headers=headers, buffered=True) assert _events(resp)[-1] == ("error", {"error": "generation failed"}) - assert state == {"recorded": 2, "failed": True} + assert state == {"spends": [21, 21], "failed": True} + db = deps.SessionLocal() + user_id = db.scalar(select(User.id).where(User.email == "stream-commit@example.com")) + assert tokens_used_today(db, user_id) == 21 # the rolled-back 21 is gone, not doubled + assert [m["role"] for m in _saved(client, headers, thread_id)] == ["user"] + left = select(func.count()).select_from(MessageSource).join(Message).where(Message.thread_id == uuid.UUID(thread_id)) + assert db.scalar(left) == 0 + + +@pytest.fixture +def kept_on_commit(client, monkeypatch): + """Sessions that keep their objects on commit, as production's SessionLocal + does (src/core/db.py). The live answer is then serialized from the list + _save_answer built, rather than re-read in the relationship's order.""" + make_session = deps.SessionLocal + + def keeping(): + session = make_session() + session.expire_on_commit = False + return session + + monkeypatch.setattr(deps, "SessionLocal", keeping) + return client + + +def test_done_numbers_sources_as_a_reload_does(kept_on_commit): + """Seals are numbered by source order, so a live answer and the same answer + reloaded must list its sources alike: most relevant first, then byte-wise + by chunk and label, whatever order retrieval gave them in.""" + client = kept_on_commit + headers, thread_id = _thread(client, "stream-order@example.com") + chunks = [ + RetrievedChunk("c_low", "low", "wiki_Low.txt", 0.31), + RetrievedChunk("c_tie", "high", "plan a - Summary of Benefits.pdf", 0.92), + RetrievedChunk("c_tie", "high", "Plan B - Summary of Benefits.pdf", 0.92), + RetrievedChunk("c_mid", "mid", "wiki_Mid.txt", 0.55), + RetrievedChunk("b_tie", "high", "wiki_Tie.txt", 0.92), + ] + done = _events(_stream(client, headers, thread_id, [Done(Answer("A.", chunks))]))[-1][1] + + def order(sources): + return [(s["chunk_id"], s["source"]) for s in sources] + + expected = [("b_tie", "wiki_Tie.txt"), ("c_tie", "Plan B - Summary of Benefits.pdf"), + ("c_tie", "plan a - Summary of Benefits.pdf"), ("c_mid", "wiki_Mid.txt"), ("c_low", "wiki_Low.txt")] + assert order(done["message"]["sources"]) == expected + assert order(_saved(client, headers, thread_id)[1]["sources"]) == expected