diff --git a/src/lembas/__init__.py b/src/lembas/__init__.py index c87f999..8495fa4 100644 --- a/src/lembas/__init__.py +++ b/src/lembas/__init__.py @@ -1,3 +1,3 @@ """LLeMbas - a Middle-earth themed web UI for OpenAI-compatible LLM endpoints.""" -__version__ = "0.6.2" +__version__ = "0.6.3" diff --git a/src/lembas/api/chats.py b/src/lembas/api/chats.py index 92299e5..ed4e691 100644 --- a/src/lembas/api/chats.py +++ b/src/lembas/api/chats.py @@ -855,19 +855,6 @@ def _step_html(message_id: str, step) -> str: ) -def _steps_tail_html(message_id: str) -> str: - """The two containers the live thinking and prose are swapped into. - - Sent as part of the `steps` frame rather than as a frame of its own, which - is how the tail clears when a round closes: what was being written is now a - step above, and re-emitting these empty is what stops it also showing below. - It means `reasoning` and `render` can keep their never-blank guard. - """ - return templates.get_template("chat/_steps_tail.html").render( - {"message": SimpleNamespace(id=message_id, reasoning_ms=0)} - ) - - def _ask_html(chat_id: str, pending) -> str: """The card asking the reader something, or nothing at all. @@ -948,14 +935,15 @@ async def _follow(chat_id: str, message_id: str) -> AsyncIterator[str]: for step in steps_service.closed_from(generation, since=marks_done): rendered.append(_step_html(message_id, step)) marks_done = len(generation.steps) - yield sse.event( - "steps", "".join(rendered) + _steps_tail_html(message_id) - ) + yield sse.event("steps", "".join(rendered)) + # Sent every pass, empty included. That is what clears the tail + # when a round closes and its contents become a step above -- + # and it is safe precisely because these carry the open tail + # only. The version that carried the whole reply had to be + # guarded, or a frame could wipe the answer. thinking_tail, text_tail = steps_service.tail(generation) - if thinking_tail: - yield sse.event("reasoning", escape_text(thinking_tail)) - if text_tail: - yield sse.event("render", render_markdown(text_tail)) + yield sse.event("reasoning", escape_text(thinking_tail)) + yield sse.event("render", render_markdown(text_tail) if text_tail else "") if generation.canvas.get("tabs"): # Guarded on truthiness, which puts this in the # reasoning/tools/render group and not the diff --git a/src/lembas/web/static/css/chat.css b/src/lembas/web/static/css/chat.css index f0972f2..0a7db63 100644 --- a/src/lembas/web/static/css/chat.css +++ b/src/lembas/web/static/css/chat.css @@ -99,6 +99,20 @@ max-width: 100%; } +/* A reply is a sequence of steps: thinking, prose, a tool call, more prose. The + gap is what separates one from the next -- without it a paragraph and the + command it led to run together and the ordering the whole thing exists for is + not legible. Each child brings its own margins, so this only has to space + them consistently. */ +.msg__steps { + display: flex; + flex-direction: column; + gap: var(--sp-2); +} +.msg__steps:empty { display: none; } +/* The blocks inside carry the rhythm; their own bottom margins would double it. */ +.msg__steps > .reasoning { margin-bottom: 0; } + .msg__error { margin: var(--sp-2) 0; align-items: flex-start; } /* --- Streaming indicator -------------------------------------------------- */ @@ -557,8 +571,13 @@ padding: var(--sp-2) 0; } /* Once the answer has content the caret carries the "still going" signal, so - the dots go, but Stop must stay reachable until the stream ends. */ -.msg__body--live:not(:empty) + .msg__waiting .dots { display: none; } + the dots go, but Stop must stay reachable until the stream ends. + + `:has()` on the bubble rather than a sibling selector: the live body and the + waiting row stopped being siblings when the reply became a sequence of steps, + and an adjacent-sibling rule that matches nothing fails by leaving the dots + pulsing beside a finished-looking answer forever. */ +.msg__main:has(.msg__body--live:not(:empty)) .msg__waiting .dots { display: none; } /* Send and Stop are one button. Which icon shows is decided here rather than in JavaScript, so the state is visible in the markup and the swap is free. */ diff --git a/src/lembas/web/templates/chat/_message.html b/src/lembas/web/templates/chat/_message.html index 24390b3..da4b602 100644 --- a/src/lembas/web/templates/chat/_message.html +++ b/src/lembas/web/templates/chat/_message.html @@ -95,16 +95,34 @@ {% endif %} {% if streaming %} - {# The reply as a sequence of steps, in the order they happened. The - closed ones are re-sent only when a round ends; the two live containers - come with them, inside `_steps.html`, which is how the tail blanks - itself. See services/steps.py. #} + {# The reply as a sequence of steps, in the order they happened. Closed + steps only, re-sent when a round ends. See services/steps.py. #}
- {% with steps = [], live = true %} - {% include "chat/_steps.html" %} - {% endwith %} -
+ sse-swap="steps" hx-swap="innerHTML"> + + {# The step still being written. SIBLINGS of the container above, never + inside it: that one is swapped whole every time a round closes, and an + `sse-swap` element nested in another is torn out and rebuilt at each + boundary with the frames aimed at it arriving in the same pass. That is + what made an agent reply render nothing from its first tool call on. + + Both frames are sent on every version bump *including empty*, which is + how the tail clears when a round closes and its contents move into a + step above. That is safe here and was not before: these carry the open + tail only, so an empty one means the tail is genuinely empty, whereas + the version that carried the whole reply would have wiped it. `steps` + is the one that must never blank now. #} +
+ + {{ icon("sparkle", "icon--sm reasoning__icon") }} + Thinking… + {{ icon("chevron-down", "icon--sm reasoning__chevron") }} + +
+
+ +
{# Where a question from the model, or a command waiting to be allowed, lands. Unlike the blocks above it this frame is sent on every version diff --git a/src/lembas/web/templates/chat/_steps.html b/src/lembas/web/templates/chat/_steps.html index 6dfc309..671170e 100644 --- a/src/lembas/web/templates/chat/_steps.html +++ b/src/lembas/web/templates/chat/_steps.html @@ -6,17 +6,19 @@ the moment the stream ends. That was the point of putting the marks on the row rather than only on the running generation. - When `live`, the two containers for the step still being written come last, and - they are part of *this* fragment rather than of `_message.html`. That is what - lets the tail clear itself: the `steps` frame is sent whenever a round closes - and re-emits them empty, so the prose and thinking that have just become a - closed step above do not also linger below. `reasoning` and `render` keep their - "never send an empty one" guard, and `metrics`, `status` and `ask` remain the - only frames allowed to blank what is on screen. + Closed steps ONLY. The step still being written lives in `_message.html`, in + two containers that are **siblings of this one, never inside it**. + + That is not tidiness, it is the whole reason this works. This container is + itself an `sse-swap` target: every time a round closes, its `innerHTML` is + replaced. Anything inside it carrying an `sse-swap` of its own is therefore + torn out and rebuilt at every round boundary — and the frames aimed at it in + the same pass have nowhere to land. Nesting them here is what made an agent + reply show nothing at all from its first tool call onwards, while an ordinary + chat was fine: an ordinary chat closes no steps, so the swap never happened. + + One `sse-swap` element must never contain another. There is a test. #} {% for step in steps %} {% include "chat/_step.html" %} {% endfor %} -{% if live %} - {% include "chat/_steps_tail.html" %} -{% endif %} diff --git a/src/lembas/web/templates/chat/_steps_tail.html b/src/lembas/web/templates/chat/_steps_tail.html deleted file mode 100644 index 612e2b5..0000000 --- a/src/lembas/web/templates/chat/_steps_tail.html +++ /dev/null @@ -1,29 +0,0 @@ -{% from "_macros.html" import icon %} -{# - The step still being written: everything past the last mark. - - These two are the only things that move at streaming speed. Everything above - them is closed and is re-sent only when a round ends, which is what keeps a - forty-round reply from re-rendering its whole transcript twelve times a second. - - Both carry the complete block each time rather than a delta -- that is what - makes reattaching to a reply in progress work at all, since a follower arriving - late has no earlier fragments to append to. - - The reasoning wrapper is static and only its body is swapped, so a reader who - opens it keeps it open for as long as this step lasts. When the round closes it - becomes a `think-…` block above and comes back collapsed; that is a real seam - and it is left visible rather than papered over with a mapping from an - ephemeral id to a permanent one. -#} -
- - {{ icon("sparkle", "icon--sm reasoning__icon") }} - Thinking… - {{ icon("chevron-down", "icon--sm reasoning__chevron") }} - -
-
- -
diff --git a/tests/test_agent_transcript.py b/tests/test_agent_transcript.py new file mode 100644 index 0000000..66091a9 --- /dev/null +++ b/tests/test_agent_transcript.py @@ -0,0 +1,234 @@ +"""An agent reply, rendered end to end. + +The steps rewrite touched the one template every bubble goes through and the +frames that fill it, and an agent reply is the case with all of it at once: +reasoning, prose, tool events, a plan card, a project listing. `test_steps.py` +covers the builder as pure functions; this covers the page. +""" + +from __future__ import annotations + +import json as _json + +import pytest +from fastapi.testclient import TestClient +from sqlalchemy import select + +from lembas.db.models import KIND_AGENT, Chat, Connection, Message, Model, SshProfile, User +from lembas.services import generation as generation_service +from lembas.services import settings_store + + +def _text_chunk(text: str) -> dict: + return {"choices": [{"delta": {"content": text}}]} + + +def _tool_chunk(name: str, arguments: str) -> dict: + return { + "choices": [ + { + "delta": { + "tool_calls": [ + { + "index": 0, + "id": "call_1", + "function": {"name": name, "arguments": arguments}, + } + ] + } + } + ] + } + + +def _stub_stream(rounds): + calls = {"n": 0} + + async def stream(_endpoint, _payload): + index = min(calls["n"], len(rounds) - 1) + calls["n"] += 1 + for chunk in rounds[index]: + yield chunk + + return stream + + +@pytest.fixture +def agent_chat(db, registered): + """An agent chat whose connection exists but has no reachable host. + + Deliberately unreachable: what is under test is the *rendering* of a reply + that called a tool, and a tool that fails still produces an event, which is + the thing the transcript has to lay out. + """ + settings_store.update( + db, + {"enabled": True, "background_enabled": True, "terminal_enabled": True}, + key=settings_store.AGENTS, + ) + user = db.scalar(select(User)) + + profile = SshProfile( + owner_id=user.id, + name="Box", + host="127.0.0.1", + port=1, + username="nobody", + host_key="ssh-ed25519 AAAA", + host_fingerprint="SHA256:x", + default_dir="/srv/project", + ) + connection = Connection(name="c", base_url="http://127.0.0.1:1", api_key_encrypted="") + db.add_all([profile, connection]) + db.commit() + db.add(Model(connection_id=connection.id, model_id="m", capabilities_json={"tools": True})) + db.commit() + + chat = Chat( + user_id=user.id, + model_id="m", + connection_id=connection.id, + kind=KIND_AGENT, + ssh_profile_id=profile.id, + project_dir="/srv/project", + agent_mode="auto", + ) + db.add(chat) + db.commit() + return chat + + +async def test_an_agent_reply_that_called_a_tool_renders( + client: TestClient, db, agent_chat, registered, monkeypatch +): + """The regression this file exists for: the bubble is rendered from the + steps now, and an agent reply is where every kind of step appears at once.""" + db.add(Message(chat_id=agent_chat.id, role="user", content="Do the thing.", complete=True)) + reply = Message(chat_id=agent_chat.id, role="assistant", content="", complete=False) + db.add(reply) + db.commit() + reply_id = reply.id + + monkeypatch.setattr( + generation_service, + "stream_chat", + _stub_stream( + [ + [ + _text_chunk("Listing the project first. "), + _tool_chunk("file_list", _json.dumps({"path": "."})), + ], + [_text_chunk("Nothing I can reach.")], + ] + ), + ) + monkeypatch.setattr("lembas.services.chat.generate_title", lambda *a, **k: "") + + generation = generation_service.Generation(chat_id=agent_chat.id, message_id=reply_id) + await generation_service._run(generation) + + db.expire_all() + stored = db.get(Message, reply_id) + assert stored.tool_calls_json, "the tool event was recorded" + assert stored.steps_json, "and so were the marks" + + page = client.get(f"/chat/{agent_chat.id}") + assert page.status_code == 200 + assert "Listing the project first" in page.text + assert "Nothing I can reach" in page.text + assert page.text.index("Listing the project first") < page.text.index("tool-activity") + + +def test_the_page_renders_before_any_reply_exists(client: TestClient, agent_chat, registered): + """An agent chat with nothing in it. The composer takes a different branch + here -- the mode select, the jobs chip -- and it is the screen somebody sees + first.""" + page = client.get(f"/chat/{agent_chat.id}") + + assert page.status_code == 200 + assert 'name="agent_mode"' in page.text + + +def test_starting_an_agent_chat_from_the_composer_renders( + client: TestClient, db, registered, agent_chat +): + """`POST /api/chats/start` returns the thread it just created. This is the + exact moment the reader reported a blank screen.""" + profile = db.scalar(select(SshProfile)) + response = client.post( + "/api/chats/start", + data={ + "content": "Do the thing.", + "kind": "agent", + "ssh_profile_id": profile.id, + "project_dir": "/srv/project", + "model_id": "m", + }, + ) + + # 204 plus HX-Redirect is the contract: the row is created here and the + # browser then navigates to it. The blank screen is on the page it lands on. + assert response.status_code == 204 + assert response.headers["HX-Redirect"].startswith("/chat/") + + landed = client.get(response.headers["HX-Redirect"]) + assert landed.status_code == 200 + assert "Do the thing." in landed.text + + +def test_the_page_renders_while_the_reply_is_still_unfinished( + client: TestClient, db, agent_chat, registered +): + """The streaming branch of the bubble, which is what somebody sees for the + whole of a long agent reply -- and what they saw blank. + + An unfinished assistant message is the *only* thing that starts a + generation, so this is also the page that sets the reply going. + """ + db.add(Message(chat_id=agent_chat.id, role="user", content="Do the thing.", complete=True)) + db.add(Message(chat_id=agent_chat.id, role="assistant", content="", complete=False)) + db.commit() + + page = client.get(f"/chat/{agent_chat.id}") + + assert page.status_code == 200 + assert "Do the thing." in page.text + assert 'sse-swap="steps"' in page.text, "the container the frames land in" + assert 'sse-swap="render"' in page.text + + +async def test_the_stream_delivers_the_steps_frame( + client: TestClient, db, agent_chat, registered, monkeypatch +): + """The SSE path with the new frame set, which is the one place the steps are + rendered without a `Message` row to hang them on.""" + db.add(Message(chat_id=agent_chat.id, role="user", content="Do it.", complete=True)) + reply = Message(chat_id=agent_chat.id, role="assistant", content="", complete=False) + db.add(reply) + db.commit() + reply_id = reply.id + + monkeypatch.setattr( + generation_service, + "stream_chat", + _stub_stream( + [ + [ + _text_chunk("Listing first. "), + _tool_chunk("file_list", _json.dumps({"path": "."})), + ], + [_text_chunk("Done.")], + ] + ), + ) + monkeypatch.setattr("lembas.services.chat.generate_title", lambda *a, **k: "") + + with client.stream( + "GET", f"/api/chats/{agent_chat.id}/messages/{reply_id}/stream" + ) as response: + assert response.status_code == 200 + body = "".join(response.iter_lines()) + + assert "event: steps" in body, "the closed steps never reached the browser" + assert "event: done" in body + assert "Listing first" in body diff --git a/tests/test_chat.py b/tests/test_chat.py index 073499e..314c149 100644 --- a/tests/test_chat.py +++ b/tests/test_chat.py @@ -877,19 +877,26 @@ def _user_id(db): return db.scalar(select(User.id)) -def test_both_branches_of_the_bubble_render_the_same_partial(): - """The streaming shell and the finished bubble are built from one builder, - so a reply cannot rearrange itself the moment the stream ends -- which is - what it used to do the other way round, three zones either side.""" +def test_both_paths_render_a_step_through_the_same_partial(): + """A live step and a stored one are the same markup, so a reply cannot + rearrange itself the moment the stream ends -- which is what it used to do + the other way round, three zones either side. + + They arrive by different routes and that is deliberate: the finished bubble + loops `_steps.html`, while the stream renders one `_step.html` at a time and + swaps the accumulated prefix in. Both bottom out in the same file, which is + the property worth pinning. + """ from pathlib import Path import lembas - source = ( - Path(lembas.__file__).parent / "web/templates/chat/_message.html" - ).read_text() + root = Path(lembas.__file__).parent + steps_partial = (root / "web/templates/chat/_steps.html").read_text() + follower = (root / "api/chats.py").read_text() - assert source.count('include "chat/_steps.html"') == 2 + assert 'include "chat/_step.html"' in steps_partial + assert 'get_template("chat/_step.html")' in follower def test_no_template_still_asks_for_a_tools_frame(): @@ -905,26 +912,60 @@ def test_no_template_still_asks_for_a_tools_frame(): assert 'sse-swap="tools"' not in path.read_text(), path -def test_the_steps_frame_carries_the_live_containers(): - """That is how the tail blanks itself. When a round closes, what was being - written becomes a step above; re-emitting these two empty is what stops it - also showing below -- and it lets `reasoning` and `render` keep their - never-send-an-empty-one guard, which is what makes reattaching work.""" - from types import SimpleNamespace +def test_no_sse_swap_element_contains_another(): + """The regression that made an agent reply render nothing at all. - from lembas.web.templating import templates + `#steps-{id}` is itself an `sse-swap` target, so its whole `innerHTML` is + replaced every time a round closes. An `sse-swap` element nested inside is + therefore torn out and rebuilt at every round boundary -- with the frames + aimed at it arriving in the same pass, at something that is no longer the + element the listener was bound to. An ordinary chat never noticed, because + it closes no steps and the swap never happens; an agent chat lost its answer + from the first tool call onwards. - html = templates.get_template("chat/_steps.html").render( - {"steps": [], "live": True, "message": SimpleNamespace(id="m1", reasoning_ms=0)} - ) + Asserted structurally rather than by rendering, so it holds for whichever + branch of the template a given reply takes. + """ + import re + from html.parser import HTMLParser + from pathlib import Path - assert 'sse-swap="reasoning"' in html - assert 'sse-swap="render"' in html + import lembas + + root = Path(lembas.__file__).parent / "web/templates" + + class Nesting(HTMLParser): + def __init__(self): + super().__init__() + self.depth = 0 + self.stack = [] + self.found = [] + + def handle_starttag(self, tag, attrs): + got = dict(attrs) + swaps = "sse-swap" in got + if swaps and self.depth: + self.found.append(got.get("sse-swap")) + if tag not in ("br", "img", "input", "hr", "meta", "link", "use"): + self.stack.append(swaps) + self.depth += 1 if swaps else 0 + + def handle_endtag(self, tag): + if self.stack and self.stack.pop(): + self.depth -= 1 + + for path in root.rglob("*.html"): + source = path.read_text() + stripped = re.sub(r"\{%.*?%\}|\{#.*?#\}", "", source, flags=re.S) + stripped = re.sub(r"\{\{.*?\}\}", "x", stripped, flags=re.S) + parser = Nesting() + parser.feed(stripped) + assert not parser.found, f"{path.name} nests sse-swap: {parser.found}" def test_the_finished_bubble_carries_no_live_containers(): - """The mirror of the above. A finished reply with an `sse-swap` in it is a - container waiting for a stream that is over.""" + """A finished reply with an `sse-swap` in it is a container waiting for a + stream that is over.""" from types import SimpleNamespace from lembas.web.templating import templates