From 4b892054a40799a495b55b3345329f55d382374a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jaroslav=20Bene=C5=A1?= Date: Sat, 1 Aug 2026 22:01:46 +0200 Subject: [PATCH] Ask several questions on one card One `ask_user` call can now carry several questions, and they come back in a single submit. Asking one at a time cost a round trip and an interruption each, and by the third you had forgotten the first. Each question becomes an item with its own key; several items share a call index, because they belong to one call and one tool turn has to answer them all. Each answer is quoted beside the question it belongs to -- with four on a card, a bare list would leave the model matching them up by position and sometimes getting it wrong. Options are radios rather than submit buttons, so picking one does not send the form while two other questions are still blank. What you type beats what you picked: someone who writes in the box after clicking an option meant the writing. `_questions_in` also reads the shapes a small model actually sends -- a bare `question` string, a list of plain strings, one object where a list belonged. Getting that wrong costs a whole round trip and shows a card saying nothing. Two test fixes, both mine. `test_posting_a_message_stores_both_turns` raced the background generation it started: against a connection that refuses instantly the reply sometimes won, writing the error and marking the row complete before the assertions could read it. And the generation registry is module-global, so a test that started a reply left an entry -- and a Task belonging to a closed event loop -- for the rest of the session. Co-Authored-By: Claude Opus 5 (1M context) --- src/lembas/api/chats.py | 29 +- src/lembas/services/generation.py | 102 ++++-- src/lembas/services/interaction.py | 51 ++- src/lembas/services/tools.py | 51 ++- src/lembas/web/static/css/chat.css | 49 ++- .../web/templates/chat/_interaction.html | 100 +++-- tests/conftest.py | 22 ++ tests/test_agent_interaction.py | 342 ++++++++++++++++-- tests/test_chat.py | 17 +- 9 files changed, 628 insertions(+), 135 deletions(-) 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()