diff --git a/CLAUDE.md b/CLAUDE.md index 59265b0..1f968ce 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -245,9 +245,51 @@ work: a follower arriving late has no earlier fragments to append to. It also means Markdown is re-rendered whole, which is required anyway -- a list or code fence is only correct once its context exists. -**Stopping sets a flag the producer checks.** `generation.request_stop()`; -whatever arrived is kept and the message is marked `stopped`, which is distinct -from `error`. In-process, so single-worker only. +**Two frames must be able to blank themselves, and the rest must not.** +`reasoning`, `tools` and `render` are only sent when they have something in +them, so a frame can never wipe what is on screen. `metrics`, `status` and +`ask` are sent on every version bump *including empty*, because each has to be +able to clear: an approval card that survived being answered would be a button +you could press twice. + +**Stopping sets a flag the producer checks -- except while it is paused.** +`generation.request_stop()`; whatever arrived is kept and the message is marked +`stopped`, distinct from `error`. In-process, so single-worker only. `cancel` is +read in exactly one place, between streamed chunks, and a reply waiting on an +approval produces no chunks -- so `request_stop` also resolves +`generation.pending`, and that is the wakeup. Without it the Stop button does +nothing at all while a card is on screen, silently. + +**Asking a person is one primitive with three uses.** +`services/interaction.py`: a command waiting to be allowed, a question the model +asked, and "this reply is waiting for you" are all *pause, render a block in the +bubble, wait for a POST, resume*. It pauses a **round, not a call** -- a round's +calls run together under a semaphore, and parking four coroutines on four +separate answers inside that gather would queue them behind each other +invisibly, and hand the reader four cards for commands whose order matters. So +one card covers everything in the round, and `_authorise` returns pre-decided +outcomes keyed by call index, which is what keeps +`zip(calls, outcomes, strict=True)` aligned. + +**One `ask_user` call may carry several questions, and they come back at once.** +Each becomes an `Item` with its own `key`; several items can share an `index` +because they belong to one call, and one tool turn answers them all with each +answer quoted beside its question. Asking one at a time would cost a round trip +and an interruption each, and answering the third would mean having forgotten +the first. `_questions_in` reads the singular form and bare strings too: a small +model sends something close to the schema rather than the schema, and getting it +wrong costs a whole round trip to show a card that says nothing. + +**A paused reply is deliberately not `done`.** That is what lets a page reload +reattach to it. Its *timeout* is what stops it lingering, not `_prune`, which +only drops finished ones -- so `approval_timeout` is clamped to at least a +minute on read, and `_prune` resolves anything whose deadline is long past as a +backstop. + +**Nothing is persisted while paused.** A restart abandons the pending question +along with the reply, and a reload starts the turn afresh -- the model asks +again. That is consistent with "a restart abandons replies in flight", but it +means an approval is not a durable record of consent. **Unread is polled, not pushed.** A browser on another chat has no connection to the one that finished. `/api/chats/unread` returns out-of-band dot spans and diff --git a/src/lembas/api/chats.py b/src/lembas/api/chats.py index e54c8ed..4f4582a 100644 --- a/src/lembas/api/chats.py +++ b/src/lembas/api/chats.py @@ -675,14 +675,19 @@ async def stop_message(db: Db, user: RequiredUser, chat_id: str, message_id: str @router.post("/{chat_id}/interaction/{interaction_id}") async def answer_interaction( + request: Request, db: Db, user: RequiredUser, chat_id: str, interaction_id: str, - choice: str = Form(""), - text: str = Form(""), ) -> Response: - """Answer a question, or allow something, that a reply is waiting on. + """Answer the questions, or allow the action, a reply is waiting on. + + The whole card comes back at once, which is why the raw form is read rather + than declared parameters: one `ask_user` call may have put four questions, + and each carries a chosen option and a box to write something else. Per + question, what was written wins over what was picked -- somebody who typed + in the box after clicking an option meant the typing. `_owned_chat` is the authorisation and it is not decoration: without it any signed-in account that guessed an id would be answering -- and later, @@ -694,8 +699,24 @@ async def answer_interaction( being told plainly. """ chat = _owned_chat(db, chat_id, user.id) + form = await request.form() + + answers: dict[str, str] = {} + for field, value in form.multi_items(): + kind, _, key = str(field).partition(".") + if not key or kind not in ("choice", "text"): + continue + written = str(value).strip() + if kind == "text" and written: + answers[key] = written + elif kind == "choice" and written: + answers.setdefault(key, written) + answered = generation_service.answer( - chat.id, interaction_id, choice=choice.strip(), text=text.strip() + chat.id, + interaction_id, + verdict=str(form.get("verdict") or "").strip(), + answers=answers, ) response = Response(status_code=status.HTTP_204_NO_CONTENT) diff --git a/src/lembas/services/generation.py b/src/lembas/services/generation.py index 9abb6f4..faa86d7 100644 --- a/src/lembas/services/generation.py +++ b/src/lembas/services/generation.py @@ -156,7 +156,13 @@ def request_stop(message_id: str) -> bool: return True -def answer(chat_id: str, interaction_id: str, *, choice: str, text: str) -> bool: +def answer( + chat_id: str, + interaction_id: str, + *, + verdict: str = "", + answers: dict[str, str] | None = None, +) -> bool: """Resolve whichever running reply is parked on this interruption. A linear scan of the registry: it holds one entry per reply in flight, and @@ -167,12 +173,12 @@ def answer(chat_id: str, interaction_id: str, *, choice: str, text: str) -> bool pending = generation.pending if generation.chat_id != chat_id or pending is None or pending.id != interaction_id: continue - outcome = choice if choice in _ANSWERS else interaction.ANSWER - return pending.resolve(outcome, text=text or choice) + outcome = verdict if verdict in _VERDICTS else interaction.ANSWER + return pending.resolve(outcome, answers=answers) return False -_ANSWERS = (interaction.ALLOW, interaction.ALLOW_ALWAYS, interaction.DENY) +_VERDICTS = (interaction.ALLOW, interaction.ALLOW_ALWAYS, interaction.DENY) def _prune() -> None: @@ -566,19 +572,51 @@ def _ask_items(context, calls: list[dict]) -> list[interaction.Item]: if not isinstance(args, dict): args = {} - options = [str(o).strip() for o in (args.get("options") or []) if str(o).strip()] - items.append( - interaction.Item( - index=index, - kind=interaction.KIND_QUESTION, - tool_name=call["name"], - title=str(args.get("question") or "").strip() or "A question for you", - options=tuple(options[: interaction.MAX_OPTIONS]), + for asked in _questions_in(args): + options = [str(o).strip() for o in (asked.get("options") or []) if str(o).strip()] + items.append( + interaction.Item( + index=index, + key=f"q{len(items)}", + kind=interaction.KIND_QUESTION, + tool_name=call["name"], + title=str(asked.get("question") or "").strip() or "A question for you", + options=tuple(options[: interaction.MAX_OPTIONS]), + ) ) - ) return items +def _questions_in(args: dict) -> list[dict]: + """The questions in one `ask_user` call, however it was spelled. + + The schema asks for a list of objects, and a capable model sends that. A + small one sends a bare `question` string, or a list of plain strings, or + one object where a list belonged -- all of which mean something obvious, so + they are read rather than refused. Getting this wrong costs a whole round + trip and produces a card saying "A question for you" and nothing else. + """ + raw = args.get("questions") + if raw is None: + raw = args.get("question") + if raw is None: + return [] + if isinstance(raw, str | dict): + raw = [raw] + if not isinstance(raw, list): + return [] + + out: list[dict] = [] + for entry in raw[: interaction.MAX_QUESTIONS]: + if isinstance(entry, str) and entry.strip(): + # A bare string, possibly alongside a sibling `options` that was + # meant to go with it -- which only makes sense for a lone question. + out.append({"question": entry, "options": args.get("options") if len(raw) == 1 else []}) + elif isinstance(entry, dict) and str(entry.get("question") or "").strip(): + out.append(entry) + return out + + async def _authorise(generation, context, calls: list[dict]) -> dict[int, ToolOutcome]: """Which of this round's calls may run, and what the others answer instead. @@ -602,18 +640,24 @@ async def _authorise(generation, context, calls: list[dict]) -> dict[int, ToolOu generation.stopped = True return {} - return {item.index: _answered(item, reply) for item in items} + # Grouped back by call, because one `ask_user` call may have carried several + # questions and the endpoint expects exactly one tool turn per call. + grouped: dict[int, list[interaction.Item]] = {} + for item in items: + grouped.setdefault(item.index, []).append(item) + return {index: _answered(asked, reply) for index, asked in grouped.items()} -def _answered(item: interaction.Item, reply: interaction.Reply) -> ToolOutcome: - """One question's answer, as the model will read it back.""" +def _answered(items: list[interaction.Item], reply: interaction.Reply) -> ToolOutcome: + """What one `ask_user` call gets back, however many questions it put.""" event = { - "name": item.tool_name, + "name": items[0].tool_name, "kind": "ask", "label": "Asked you", - "query": item.title, + "query": "; ".join(item.title for item in items), "results": [], } + if reply.outcome == interaction.EXPIRED: return ToolOutcome( "They did not answer. Carry on as best you can without it, or say " @@ -621,16 +665,26 @@ def _answered(item: interaction.Item, reply: interaction.Reply) -> ToolOutcome: {**event, "status": "error", "error": "No answer.", "text": ""}, ) - answer_text = reply.text.strip() - if not answer_text: + answered = [(item, reply.answer_to(item)) for item in items] + given = [(item, text) for item, text in answered if text] + if not given: return ToolOutcome( "They closed the question without answering.", {**event, "status": "error", "error": "No answer.", "text": ""}, ) - return ToolOutcome( - f"They answered: {answer_text}", - {**event, "status": "ok", "text": answer_text}, - ) + + # Each answer is quoted next to the question it belongs to. With four + # questions on one card, a bare list of answers would leave the model + # matching them up by position and sometimes getting it wrong. + lines = [f"{item.title}\n → {text}" for item, text in given] + skipped = [item for item, text in answered if not text] + if skipped: + lines.append( + "They left unanswered: " + "; ".join(item.title for item in skipped) + ) + + body = "\n\n".join(lines) + return ToolOutcome(f"They answered:\n\n{body}", {**event, "status": "ok", "text": body}) async def _run_calls( diff --git a/src/lembas/services/interaction.py b/src/lembas/services/interaction.py index f0530fd..86cf28a 100644 --- a/src/lembas/services/interaction.py +++ b/src/lembas/services/interaction.py @@ -49,18 +49,29 @@ PERMITTED = (ALLOW, ALLOW_ALWAYS) # A card offering more than this many buttons is a card nobody reads. MAX_OPTIONS = 6 +# And more than this many questions at once is a form, not a conversation. A +# model that wants twenty answers should ask for four and then ask again with +# what it learned. +MAX_QUESTIONS = 8 + @dataclass(frozen=True) class Item: - """One thing being asked about. + """One thing being asked about: a single question, or one command. - `index` is the position of the call in its round, so a decision can be - matched back to the call it was about -- the tool turns have to line up with - the assistant turn's `tool_calls` or an endpoint pairs the wrong result with - the right id. + `index` is the position of the *call* in its round, so an answer can be + matched back to the call it belongs to -- the tool turns have to line up + with the assistant turn's `tool_calls`, or an endpoint pairs the wrong + result with the right id. Several items can share an index, because one + `ask_user` call may carry several questions. + + `key` identifies this item within the card, and is what the form field is + named after. Stable and opaque: a question's own text would make a terrible + field name, and its position alone would collide across calls. """ index: int + key: str kind: str tool_name: str title: str @@ -83,18 +94,7 @@ class Interruption: def kind(self) -> str: return KIND_QUESTION if any(i.kind == KIND_QUESTION for i in self.items) else KIND_APPROVAL - @property - def options(self) -> tuple[str, ...]: - for item in self.items: - if item.options: - return item.options - return () - - @property - def allow_free_text(self) -> bool: - return any(item.allow_free_text for item in self.items) - - def resolve(self, outcome: str, *, text: str = "") -> bool: + def resolve(self, outcome: str, *, answers: dict[str, str] | None = None) -> bool: """Complete this pause. Idempotent -- a second answer is ignored. Returns whether this call was the one that answered it, which is what @@ -103,14 +103,21 @@ class Interruption: """ if self._future is None or self._future.done(): return False - self._future.set_result(Reply(outcome=outcome, text=text)) + self._future.set_result(Reply(outcome=outcome, answers=dict(answers or {}))) return True @dataclass(frozen=True) class Reply: + """How a card was answered. + + `answers` is keyed by `Item.key`, so a card carrying four questions comes + back as four answers in one go. An approval carries none: the verdict is + the whole of it. + """ + outcome: str - text: str = "" + answers: dict[str, str] = field(default_factory=dict) @property def permitted(self) -> bool: @@ -121,6 +128,9 @@ class Reply: """Whether this outcome means the whole reply should stop.""" return self.outcome == CANCELLED + def answer_to(self, item: Item) -> str: + return (self.answers.get(item.key) or "").strip() + def build( interaction_id: str, items: list[Item] | tuple[Item, ...], *, timeout: float @@ -163,7 +173,7 @@ def summarise(items: tuple[Item, ...]) -> str: if not items: return "" if items[0].kind == KIND_QUESTION: - return "Waiting for your answer…" + return "Waiting for your answer…" if len(items) == 1 else "Waiting for your answers…" if len(items) == 1: return f"Waiting for you to allow {items[0].tool_name}…" return f"Waiting for you to allow {len(items)} actions…" @@ -179,6 +189,7 @@ __all__ = [ "KIND_APPROVAL", "KIND_QUESTION", "MAX_OPTIONS", + "MAX_QUESTIONS", "PERMITTED", "Interruption", "Item", diff --git a/src/lembas/services/tools.py b/src/lembas/services/tools.py index 18863c4..e2583a2 100644 --- a/src/lembas/services/tools.py +++ b/src/lembas/services/tools.py @@ -750,31 +750,48 @@ REGISTRY: dict[str, ToolDef] = { name="ask_user", family=FAMILY_ASK, description=( - "Ask the person you are talking to a question, and wait for their " - "answer before going on. Use it when you genuinely need a decision " - "only they can make — which of several approaches to take, a " - "detail you cannot infer, permission for something consequential. " - "Offer options when there is a small set of sensible answers; they " - "can always type something else instead. Do not use it for " - "anything you can work out yourself, and never ask for a password, " - "a key or any other secret." + "Ask the person you are talking to one or more questions, and wait " + "for their answers before going on. Use it when you genuinely need " + "a decision only they can make — which of several approaches to " + "take, a detail you cannot infer, permission for something " + "consequential. Offer options when there is a small set of " + "sensible answers; they can always write their own instead. " + "\n\n" + "Ask everything you need in ONE call: they answer the whole card at " + "once and it costs them a single interruption, where asking twice " + "in a row costs two. Do not use it for anything you can work out " + "yourself, and never ask for a password, a key or any other secret." ), parameters=_object( { - "question": { - **_STRING, - "description": "One clear question, in plain language.", - }, - "options": { + "questions": { "type": "array", - "items": _STRING, "description": ( - "Up to six answers to offer as buttons. Optional; they " - "can always write their own." + "The questions to put, answered together. Ask up to " + "about four at a time; more than that is a form, not a " + "conversation." ), + "items": { + "type": "object", + "properties": { + "question": { + **_STRING, + "description": "One question, in plain language.", + }, + "options": { + "type": "array", + "items": _STRING, + "description": ( + "Up to six answers to offer for this " + "question. Optional." + ), + }, + }, + "required": ["question"], + }, }, }, - ["question"], + ["questions"], ), # Never resolved by this runner. The reader answers it, in every # mode, and the loop turns their answer into the outcome -- see diff --git a/src/lembas/web/static/css/chat.css b/src/lembas/web/static/css/chat.css index 8bebd9d..86a7ecb 100644 --- a/src/lembas/web/static/css/chat.css +++ b/src/lembas/web/static/css/chat.css @@ -411,8 +411,53 @@ text-transform: uppercase; letter-spacing: 0.06em; } -.interaction__item { display: flex; flex-direction: column; gap: var(--sp-2); } -.interaction__title { margin: 0; color: var(--ink); font-weight: 500; } +.interaction__form { display: flex; flex-direction: column; gap: var(--sp-4); } +/* One block per question. Several go on one card and submit together. */ +.interaction__question { + display: flex; + flex-direction: column; + gap: var(--sp-2); + margin: 0; + padding: 0; + border: 0; + min-width: 0; +} +.interaction__question + .interaction__question { + padding-top: var(--sp-4); + border-top: 1px solid var(--border); +} +.interaction__title { margin: 0; padding: 0; color: var(--ink); font-weight: 500; } +.interaction__options { display: flex; flex-wrap: wrap; gap: var(--sp-2); } +.interaction__note { color: var(--ink-faint); font-size: var(--text-xs); } + +/* An option. A radio, so picking one unpicks the last, but shaped like the + button it reads as. */ +.chip { position: relative; display: inline-flex; } +.chip input { + position: absolute; + inset: 0; + opacity: 0; + cursor: pointer; + margin: 0; +} +.chip span { + display: inline-flex; + align-items: center; + min-height: var(--control-h); + padding: 0 var(--sp-3); + border: 1px solid var(--border-strong); + border-radius: var(--radius-sm); + background: var(--surface-raised); + color: var(--ink-muted); + font-size: var(--text-sm); +} +.chip input:hover + span { background: var(--surface-hover); color: var(--ink); } +.chip input:checked + span { + border-color: var(--accent); + background: var(--surface-active); + color: var(--ink); +} +.chip input:focus-visible + span { outline: 2px solid var(--accent); outline-offset: 2px; } .interaction__detail { margin: 0; padding: var(--sp-3); diff --git a/src/lembas/web/templates/chat/_interaction.html b/src/lembas/web/templates/chat/_interaction.html index ec7f2ac..67b64f0 100644 --- a/src/lembas/web/templates/chat/_interaction.html +++ b/src/lembas/web/templates/chat/_interaction.html @@ -2,9 +2,9 @@ {# The reply has stopped and is waiting for you. - Two shapes, one mechanism: a question the model asked, and (later) a command + Two shapes, one mechanism: questions the model asked, and (later) a command waiting to be allowed. Everything shown here is model output and is escaped - accordingly -- the question text, the options on the buttons and the command + accordingly -- the questions, the options on the buttons and the command itself all came from a model that may have been reading somebody else's file a moment ago. @@ -12,6 +12,10 @@ asking. A question that looks like it came from the application is a question people answer with things they would not tell a chatbot. + Several questions go on ONE card and come back in ONE submit. Asking them one + at a time would cost a round trip and an interruption each, and answering the + third would mean having forgotten the first. + hx-swap="none" because the SSE stream clears this card the moment the answer lands; swapping a response in here would fight it. #} @@ -21,47 +25,71 @@ The model is asking you

- {% for item in ask.items %} -
-

{{ item.title }}

- {% if item.detail %} - 
{{ item.detail }}
- {% endif %} - {% if item.reason %} -

{{ item.reason }}

- {% endif %} -
- {% endfor %} - -
+ {% if ask.kind == "question" %} - {% for option in ask.options %} - + {% for item in ask.items %} +
+ {{ item.title }} + + {% if item.options %} +
+ {% for option in item.options %} + + {% endfor %} +
+ {% endif %} + + {% if item.allow_free_text %} + {# Never type="password". A model talked into asking for a credential + must not be handed a field that looks built for one, and a chat + transcript is not a place to keep secrets. #} + + {% endif %} +
{% endfor %} - {% if ask.allow_free_text %} - {# Never type="password". A model talked into asking for a credential - must not be handed a field that looks built for one, and a transcript - is not a place to put secrets. #} -
- + +
+ + {%- if ask.items | length > 1 %}All of them at once. {% endif -%} + Leave any blank to skip it. + +
+ + {% else %} + {% for item in ask.items %} +
+

{{ item.title }}

+ {% if item.detail %} + 
{{ item.detail }}
+ {% endif %} + {% if item.reason %} +

{{ item.reason }}

+ {% endif %} +
+ {% endfor %} + +
+ + +
- {% endif %} - {% else %} - - - {% endif %}
diff --git a/tests/conftest.py b/tests/conftest.py index 0a75a6a..94ccc13 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -58,6 +58,28 @@ def fresh_database(tmp_path: Path) -> Iterator[None]: reset_engine() +@pytest.fixture(autouse=True) +def fresh_generation_registry() -> Iterator[None]: + """Empty the in-flight reply registry between tests. + + `_RUNNING` and `_TASKS` are module-level dicts, so a test that starts a + reply and does not wait for it leaves an entry behind for the rest of the + session -- holding a Generation, and a Task belonging to an event loop that + has since closed. `_prune()` will not clear it either: it only drops + generations that have finished, and it runs on every `ensure()`. + + Cheap, and it keeps a test that posts a message from meeting the leftovers + of one that asked a question. + """ + from lembas.services import generation as generation_service + + generation_service._RUNNING.clear() + generation_service._TASKS.clear() + yield + generation_service._RUNNING.clear() + generation_service._TASKS.clear() + + @pytest.fixture def db() -> Iterator[Session]: session = get_session_factory()() diff --git a/tests/test_agent_interaction.py b/tests/test_agent_interaction.py index 2778d7b..b560877 100644 --- a/tests/test_agent_interaction.py +++ b/tests/test_agent_interaction.py @@ -137,10 +137,10 @@ async def test_the_reply_pauses_and_the_card_describes_the_question(scripted): pending = await _until_paused(generation) assert pending.kind == interaction.KIND_QUESTION assert pending.items[0].title == "Tea or coffee?" - assert pending.options == ("Tea", "Coffee") + assert pending.items[0].options == ("Tea", "Coffee") assert "Waiting for your answer" in generation.status - pending.resolve(interaction.ANSWER, text="Tea") + pending.resolve(interaction.ANSWER, answers={"q0": "Tea"}) await task @@ -149,7 +149,7 @@ async def test_the_answer_reaches_the_model_as_a_tool_result(scripted): task = asyncio.create_task(generation_service._run(generation)) pending = await _until_paused(generation) - pending.resolve(interaction.ANSWER, text="Coffee, please") + pending.resolve(interaction.ANSWER, answers={"q0": "Coffee, please"}) await task turns = [m for m in payloads[1]["messages"] if m.get("role") == "tool"] @@ -164,7 +164,7 @@ async def test_the_card_is_cleared_once_it_is_answered(scripted): pending = await _until_paused(generation) version = generation.version - pending.resolve(interaction.ANSWER, text="Tea") + pending.resolve(interaction.ANSWER, answers={"q0": "Tea"}) await task assert generation.pending is None @@ -176,13 +176,13 @@ async def test_the_transcript_keeps_what_was_asked_and_answered(scripted): task = asyncio.create_task(generation_service._run(generation)) pending = await _until_paused(generation) - pending.resolve(interaction.ANSWER, text="Tea") + pending.resolve(interaction.ANSWER, answers={"q0": "Tea"}) await task event = generation.tool_events[0] assert event["kind"] == "ask" assert event["query"] == "Tea or coffee?" - assert event["text"] == "Tea" + assert "Tea" in event["text"] # --- Stop, while nothing is streaming ---------------------------------------- @@ -249,9 +249,9 @@ def test_resolving_twice_only_counts_once(): async def go(): pause = interaction.build("abc", [_item()], timeout=5) - assert pause.resolve(interaction.ANSWER, text="first") is True - assert pause.resolve(interaction.ANSWER, text="second") is False - assert (await pause._future).text == "first" + assert pause.resolve(interaction.ANSWER, answers={"q0": "first"}) is True + assert pause.resolve(interaction.ANSWER, answers={"q0": "second"}) is False + assert (await pause._future).answers == {"q0": "first"} asyncio.run(go()) @@ -261,12 +261,14 @@ def test_an_interruption_with_no_future_cannot_be_resolved(): assert pause.resolve(interaction.ANSWER) is False -def _item() -> interaction.Item: +def _item(key: str = "q0", *, title: str = "Tea or coffee?", **kwargs) -> interaction.Item: return interaction.Item( index=0, + key=key, kind=interaction.KIND_QUESTION, tool_name="ask_user", - title="Tea or coffee?", + title=title, + **kwargs, ) @@ -281,8 +283,8 @@ async def test_answer_finds_the_pause_and_resolves_it(db, user_id): generation.pending = interaction.build("pause-1", [_item()], timeout=30) generation_service._RUNNING[message_id] = generation try: - assert generation_service.answer(chat_id, "pause-1", choice="Tea", text="") is True - assert (await generation.pending._future).text == "Tea" + assert generation_service.answer(chat_id, "pause-1", answers={"q0": "Tea"}) is True + assert (await generation.pending._future).answers == {"q0": "Tea"} finally: generation_service._RUNNING.pop(message_id, None) @@ -295,7 +297,7 @@ async def test_answer_ignores_a_pause_in_another_chat(db, user_id): generation.pending = interaction.build("pause-1", [_item()], timeout=30) generation_service._RUNNING[message_id] = generation try: - assert generation_service.answer("another-chat", "pause-1", choice="x", text="") is False + assert generation_service.answer("another-chat", "pause-1", answers={"q0": "x"}) is False assert not generation.pending._future.done() finally: generation_service._RUNNING.pop(message_id, None) @@ -372,13 +374,7 @@ def test_the_card_shows_the_question_and_its_options(): pause = interaction.Interruption( id="p1", items=( - interaction.Item( - index=0, - kind=interaction.KIND_QUESTION, - tool_name="ask_user", - title="Tea or coffee?", - options=("Tea", "Coffee"), - ), + _item(title="Tea or coffee?", options=("Tea", "Coffee")), ), ) html = _render(pause) @@ -394,12 +390,7 @@ def test_the_card_never_offers_a_password_field(): pause = interaction.Interruption( id="p1", items=( - interaction.Item( - index=0, - kind=interaction.KIND_QUESTION, - tool_name="ask_user", - title="Confirm your password to continue:", - ), + _item(title="Confirm your password to continue:"), ), ) html = _render(pause) @@ -413,10 +404,7 @@ def test_everything_on_the_card_is_escaped(): pause = interaction.Interruption( id="p1", items=( - interaction.Item( - index=0, - kind=interaction.KIND_QUESTION, - tool_name="ask_user", + _item( title="", detail="rm -rf / ',), @@ -427,3 +415,295 @@ def test_everything_on_the_card_is_escaped(): assert "" not in html assert "<img" in html + + +# --- Several questions, one card, one submit --------------------------------- +def _multi_chunk(questions: list[dict], *, call_id="c1", index=0): + import json as _json + + return { + "choices": [ + { + "delta": { + "tool_calls": [ + { + "index": index, + "id": call_id, + "function": { + "name": "ask_user", + "arguments": _json.dumps({"questions": questions}), + }, + } + ] + } + } + ] + } + + +async def test_several_questions_arrive_on_one_card(db, user_id, monkeypatch): + chat_id, message_id = _chat_that_can_ask(db, user_id) + monkeypatch.setattr( + generation_service, + "stream_chat", + _stub_stream( + [ + [ + _multi_chunk( + [ + {"question": "Which database?", "options": ["SQLite", "Postgres"]}, + {"question": "Which port?"}, + {"question": "Deploy now?", "options": ["Yes", "Later"]}, + ] + ) + ], + [_text_chunk("Understood.")], + ], + [], + ), + ) + + generation = generation_service.Generation(chat_id=chat_id, message_id=message_id) + task = asyncio.create_task(generation_service._run(generation)) + pending = await _until_paused(generation) + + assert len(pending.items) == 3 + assert [i.title for i in pending.items] == ["Which database?", "Which port?", "Deploy now?"] + assert [i.key for i in pending.items] == ["q0", "q1", "q2"] + assert pending.items[1].options == (), "a question may have no options at all" + assert "Waiting for your answers" in generation.status + + pending.resolve(interaction.ANSWER, answers={"q0": "Postgres", "q1": "5433", "q2": "Later"}) + await task + + +async def test_all_the_answers_come_back_in_one_tool_turn(db, user_id, monkeypatch): + """One call, one turn -- however many questions it carried. The endpoint + expects exactly one tool result per tool_call_id.""" + chat_id, message_id = _chat_that_can_ask(db, user_id) + payloads: list[dict] = [] + monkeypatch.setattr( + generation_service, + "stream_chat", + _stub_stream( + [ + [_multi_chunk([{"question": "Which database?"}, {"question": "Which port?"}])], + [_text_chunk("Understood.")], + ], + payloads, + ), + ) + + generation = generation_service.Generation(chat_id=chat_id, message_id=message_id) + task = asyncio.create_task(generation_service._run(generation)) + pending = await _until_paused(generation) + pending.resolve(interaction.ANSWER, answers={"q0": "Postgres", "q1": "5433"}) + await task + + turns = [m for m in payloads[1]["messages"] if m.get("role") == "tool"] + assert len(turns) == 1 + content = turns[0]["content"] + # Each answer is quoted beside its own question, so the model is not left + # matching them up by position. + assert "Which database?" in content and "Postgres" in content + assert "Which port?" in content and "5433" in content + assert content.index("Which database?") < content.index("Which port?") + + +async def test_a_question_left_blank_is_reported_as_skipped(db, user_id, monkeypatch): + chat_id, message_id = _chat_that_can_ask(db, user_id) + payloads: list[dict] = [] + monkeypatch.setattr( + generation_service, + "stream_chat", + _stub_stream( + [ + [_multi_chunk([{"question": "Which database?"}, {"question": "Which port?"}])], + [_text_chunk("Understood.")], + ], + payloads, + ), + ) + + generation = generation_service.Generation(chat_id=chat_id, message_id=message_id) + task = asyncio.create_task(generation_service._run(generation)) + pending = await _until_paused(generation) + pending.resolve(interaction.ANSWER, answers={"q0": "Postgres", "q1": ""}) + await task + + content = [m for m in payloads[1]["messages"] if m.get("role") == "tool"][0]["content"] + assert "Postgres" in content + assert "left unanswered" in content and "Which port?" in content + + +async def test_two_ask_calls_in_one_round_share_a_card_but_answer_separately( + db, user_id, monkeypatch +): + """One card, because the reader should be interrupted once -- but two tool + turns, because there were two calls.""" + import json as _json + + chat_id, message_id = _chat_that_can_ask(db, user_id) + payloads: list[dict] = [] + both = { + "choices": [ + { + "delta": { + "tool_calls": [ + { + "index": 0, + "id": "a", + "function": { + "name": "ask_user", + "arguments": _json.dumps({"questions": [{"question": "First?"}]}), + }, + }, + { + "index": 1, + "id": "b", + "function": { + "name": "ask_user", + "arguments": _json.dumps({"questions": [{"question": "Second?"}]}), + }, + }, + ] + } + } + ] + } + monkeypatch.setattr( + generation_service, + "stream_chat", + _stub_stream([[both], [_text_chunk("Understood.")]], payloads), + ) + + generation = generation_service.Generation(chat_id=chat_id, message_id=message_id) + task = asyncio.create_task(generation_service._run(generation)) + pending = await _until_paused(generation) + + assert len(pending.items) == 2 + assert {i.index for i in pending.items} == {0, 1}, "one item per call" + assert [i.key for i in pending.items] == ["q0", "q1"], "keys are unique across calls" + + pending.resolve(interaction.ANSWER, answers={"q0": "one", "q1": "two"}) + await task + + turns = [m for m in payloads[1]["messages"] if m.get("role") == "tool"] + assert [t["tool_call_id"] for t in turns] == ["a", "b"] + assert "one" in turns[0]["content"] and "two" in turns[1]["content"] + + +# --- Whatever shape the model actually emits ---------------------------------- +@pytest.mark.parametrize( + ("args", "expected"), + [ + ({"questions": [{"question": "A"}, {"question": "B"}]}, ["A", "B"]), + ({"question": "A"}, ["A"]), # the singular form + ({"questions": "A"}, ["A"]), # a string where a list belonged + ({"questions": ["A", "B"]}, ["A", "B"]), # bare strings + ({"questions": {"question": "A"}}, ["A"]), # one object, not wrapped + ({"questions": [{"nope": 1}, {"question": "B"}]}, ["B"]), # junk is dropped + ({}, []), + ({"questions": []}, []), + ], +) +def test_the_questions_are_read_however_they_were_spelled(args, expected): + """A capable model sends the schema. A small one sends something close, and + getting it wrong costs a round trip and shows a card saying nothing.""" + asked = generation_service._questions_in(args) + assert [q["question"] for q in asked] == expected + + +def test_a_lone_bare_question_keeps_a_sibling_options_list(): + asked = generation_service._questions_in({"questions": ["Tea or coffee?"], "options": ["Tea"]}) + assert asked[0]["options"] == ["Tea"] + + +def test_too_many_questions_are_cut_off(): + args = {"questions": [{"question": f"Q{i}"} for i in range(20)]} + assert len(generation_service._questions_in(args)) == interaction.MAX_QUESTIONS + + +def test_the_card_renders_every_question_with_its_own_fields(): + pause = interaction.Interruption( + id="p1", + items=( + _item("q0", title="Which database?", options=("SQLite", "Postgres")), + _item("q1", title="Which port?"), + ), + ) + html = _render(pause) + + assert "Which database?" in html and "Which port?" in html + # Radios rather than submit buttons: picking one must not send the form + # while two other questions are still blank. + assert 'type="radio" name="choice.q0" value="SQLite"' in html + assert 'name="text.q0"' in html and 'name="text.q1"' in html + # A question with no options still gets somewhere to write. + assert 'name="choice.q1"' not in html + assert html.count("Send answers") == 1, "one submit for the whole card" + + +def test_a_single_question_says_answer_rather_than_send_answers(): + html = _render(interaction.Interruption(id="p1", items=(_item(),))) + assert "Send answers" not in html + assert ">\n Answer" in html or "Answer" in html + + +def test_the_endpoint_gathers_every_answer_at_once(client, db, registered, user_id): + """The whole card in one POST -- what was typed beating what was picked.""" + chat_id, message_id = _chat_that_can_ask(db, user_id) + seen: dict = {} + + def capture(chat, interaction_id, *, verdict="", answers=None): + seen["chat"] = chat + seen["id"] = interaction_id + seen["verdict"] = verdict + seen["answers"] = answers + return True + + from lembas.api import chats as chats_api + + original = chats_api.generation_service.answer + chats_api.generation_service.answer = capture + try: + response = client.post( + f"/api/chats/{chat_id}/interaction/p1", + data={ + "choice.q0": "Postgres", + "text.q0": "", + "choice.q1": "Yes", + "text.q1": "actually, later", + "text.q2": "5433", + }, + ) + assert response.status_code == 204 + finally: + chats_api.generation_service.answer = original + + assert seen["answers"] == { + "q0": "Postgres", # picked, nothing written + "q1": "actually, later", # written wins over picked + "q2": "5433", # written, nothing to pick + } + assert seen["verdict"] == "" + assert message_id # the chat was real + + +def test_the_endpoint_passes_a_verdict_through_untouched(client, db, registered, user_id): + chat_id, _message_id = _chat_that_can_ask(db, user_id) + seen: dict = {} + + from lembas.api import chats as chats_api + + original = chats_api.generation_service.answer + chats_api.generation_service.answer = lambda c, i, *, verdict="", answers=None: ( + seen.update(verdict=verdict, answers=answers) or True + ) + try: + client.post(f"/api/chats/{chat_id}/interaction/p1", data={"verdict": "allow"}) + finally: + chats_api.generation_service.answer = original + + assert seen["verdict"] == "allow" + assert seen["answers"] == {} diff --git a/tests/test_chat.py b/tests/test_chat.py index e23e01f..a69c9ee 100644 --- a/tests/test_chat.py +++ b/tests/test_chat.py @@ -236,7 +236,22 @@ def test_starting_a_chat_ignores_a_model_you_cannot_reach(client: TestClient, db assert db.scalar(select(Chat)).model_id == "test-model" -def test_posting_a_message_stores_both_turns(client: TestClient, db, registered, make_chat): +def test_posting_a_message_stores_both_turns( + client: TestClient, db, registered, make_chat, monkeypatch +): + """What the route itself does, with no reply running behind it. + + `ensure` is stubbed out because the generation is a background task: it + would race this test to the database, and against a connection that + refuses instantly it sometimes wins -- writing the error and marking the + row complete before the assertions below can read it. What the route + guarantees is the pair of rows and the streaming shell; whether a reply has + got anywhere yet is a different test's business. + """ + from lembas.services import generation as generation_service + + monkeypatch.setattr(generation_service, "ensure", lambda *a, **k: None) + _add_connection(db) chat_id = make_chat()