diff --git a/src/lembas/services/agent/tools.py b/src/lembas/services/agent/tools.py index 482f835..e4852dd 100644 --- a/src/lembas/services/agent/tools.py +++ b/src/lembas/services/agent/tools.py @@ -42,16 +42,31 @@ FAMILY_AGENT = "agent" # stored on every message forever. MAX_EVENT_CHARS = 4000 +# And how much of a diff. Same reasoning as the constant above and the same +# ceiling in spirit: a generated file's diff can be larger than the file, and +# this one is stored on the row forever and re-parsed on every page load. +MAX_DIFF_LINES = 200 + _STRING = {"type": "string"} def _event(name: str, context: AgentContext, summary: str, **extra: Any) -> dict[str, Any]: + """One line in the transcript for one call. + + No `label`. What a tool is called is decided by `services/tool_labels.py`, + for every tool at once -- this used to write the SSH profile's name here, so + a bubble said "homeserver · ls -la" and named the machine rather than the + thing that was done. The machine is a fact about *where*, so it belongs with + the directory in `detail`, which the template already renders in the body. + """ + where = context.label + if context.project_dir: + where = f"{where}:{context.project_dir}" return { "name": name, "kind": "agent", - "label": f"{context.label}", "query": summary, - "detail": context.project_dir or "", + "detail": where, "results": [], **extra, } diff --git a/src/lembas/services/generation.py b/src/lembas/services/generation.py index 2d18928..06c01cc 100644 --- a/src/lembas/services/generation.py +++ b/src/lembas/services/generation.py @@ -30,7 +30,7 @@ from lembas.db.models import KIND_AGENT, ROLE_ASSISTANT, ROLE_USER, Chat, Messag from lembas.db.session import session_scope from lembas.services import chat as chat_service from lembas.services import compaction as compaction_service -from lembas.services import interaction, tokens +from lembas.services import interaction, tokens, tool_labels from lembas.services import metrics as metrics_service from lembas.services import prompts as prompts_service from lembas.services import tools as tools_service @@ -721,6 +721,26 @@ def _gave_up(generation, why: str) -> None: generation.touch() +def _written(generation: Generation) -> int: + """How much this reply has written so far, in tokens, reported or estimated. + + Both, because neither alone is enough. `completion_tokens` is only populated + when the endpoint sends a usage block, and a good half of the ones this + talks to -- llama.cpp, Ollama and friends -- never do; the fallback estimate + is otherwise computed once, in `_run`'s `finally:`, long after the loop that + needs it. A ceiling reading only the reported figure would work on OpenAI + and silently do nothing everywhere else, which is the worst kind of limit: + one that looks configured. + + Reasoning counts. It was generated and it was paid for, even though it is + deliberately never replayed as context. + """ + return max( + generation.completion_tokens, + tokens.estimate(generation.text + generation.thinking), + ) + + def _tool_status(calls: list[dict]) -> str: """What to show while tools run. @@ -728,7 +748,7 @@ def _tool_status(calls: list[dict]) -> str: nothing streaming, and a silent pause is exactly what a hang looks like. """ if len(calls) == 1: - return f"Running {calls[0]['name']}…" + return f"Running {tool_labels.label_for(calls[0]['name'])}…" return f"Running {len(calls)} tools…" @@ -746,17 +766,12 @@ def _describe(name: str, args: dict) -> tuple[str, str]: The detail is the thing being agreed to -- the command line, the path -- and is shown verbatim and escaped. A summary that paraphrased it would be a card approving something other than what runs. + + Delegated to services/tool_labels.py, which the transcript and the status + line read too. This used to be a hand-written if-chain and was the fourth + place with its own wording for the same tool. """ - if name == "shell_run": - return "Run a command", str(args.get("command") or "") - if name == "file_write": - return "Write a file", str(args.get("path") or "") - if name == "file_read": - return "Read a file", str(args.get("path") or "") - if name == "file_list": - return "List a directory", str(args.get("path") or "") - detail = ", ".join(f"{k}={v!r}" for k, v in list(args.items())[:4]) - return f"Use {name}", detail[:400] + return tool_labels.describe(name, args) def _approvals(context, calls: list[dict]) -> list[interaction.Item]: diff --git a/src/lembas/services/tool_labels.py b/src/lembas/services/tool_labels.py new file mode 100644 index 0000000..6f088ca --- /dev/null +++ b/src/lembas/services/tool_labels.py @@ -0,0 +1,208 @@ +"""What a tool is called in the interface, and what it looks like. + +Four places have to agree about one tool, and for the whole life of the feature +they did not: + +- the transcript (`chat/_tool_activity.html`) showed the SSH profile's name for + an agent tool -- "homeserver · ls -la", naming the machine rather than the + thing that was done -- and the raw function name for everything else, so a + saved memory read `memory_add`; +- the status line while a round runs said "Running shell_run…"; +- the approval card had its own hand-written if-chain; +- and nothing checked that any of the three matched. + +So the table lives here and each of them reads it. + +`LABELS` and `ACTIONS` are deliberately different words for the same tool, the +same way `policy.MODE_HINTS` and `policy.MODE_GUIDANCE` are. A label is a noun +phrase in a list of things that happened; an approval card is a sentence +somebody is agreeing to, and "Bash" is not one. + +**The precedence is inverted on purpose, and that is the whole design.** +Tool events are persisted in `Message.tool_calls_json`, so every row written +before today already carries `label: "homeserver"`. A resolver that preferred +the stored label would fix nothing for any transcript that already exists. So +a name this module knows about resolves from the static table and the stored +label is ignored; a name it does not know -- a custom HTTP tool, an MCP tool, +whose labels are per row and cannot be tabulated -- keeps its own. One rule, +both cases correct. + +Resolved **without a database**. It is called once per rendered event, and +reaching for `tools.registry(db)` from a Jinja global would be two table scans +per bubble. +""" + +from __future__ import annotations + +from typing import Any + +# What the transcript calls each tool. Kept in the same order as the families +# in `services/tools.py` so that adding one has an obvious home. +LABELS: dict[str, str] = { + # Acting on the machine an agent chat is pointed at. + "shell_run": "Bash", + "file_read": "Read", + "file_write": "Write", + "file_edit": "Update", + "file_list": "List", + "plan_submit": "Plan", + "plan_update": "Plan updated", + # The web. + "web_search": "Web search", + "fetch": "Fetch", + # The library. + "knowledge_search": "Knowledge searched", + "knowledge_get": "Document read", + "notes_search": "Notes searched", + "notes_get": "Note read", + "notes_create": "Note written", + "notes_edit": "Note updated", + "notes_delete": "Note deleted", + "memory_add": "Memory saved", + "memory_forget": "Memory removed", + "skill_get": "Skill read", + "skill_create": "Skill written", + "skill_edit": "Skill updated", + # Stopping to ask. + "ask_user": "Asked you", +} + +# A symbol id from templates/partials/icons.html. Everything used to be the +# sparkle, which said only "a model did something". +ICONS: dict[str, str] = { + "shell_run": "terminal", + "file_read": "file-text", + "file_write": "pencil", + "file_edit": "diff", + "file_list": "folder", + "plan_submit": "check", + "plan_update": "check", + "web_search": "globe", + "fetch": "link", + "knowledge_search": "archive", + "knowledge_get": "file-text", + "notes_search": "search", + "notes_get": "file-text", + "notes_create": "pencil", + "notes_edit": "pencil", + "notes_delete": "trash", + "memory_add": "star", + "memory_forget": "trash", + "skill_get": "sparkle", + "skill_create": "sparkle", + "skill_edit": "sparkle", + "ask_user": "chat", +} + +# The icon for an event whose tool is not in the table -- a custom HTTP tool, an +# MCP tool, or a row written before `kind` existed. +KIND_ICONS: dict[str, str] = { + "search": "globe", + "fetch": "link", + "custom": "link", + "mcp": "server", +} +FALLBACK_ICON = "sparkle" + +# What an approval card is headed. A sentence somebody agrees to, in the +# imperative, because that is what pressing the button does. +ACTIONS: dict[str, str] = { + "shell_run": "Run a command", + "file_read": "Read a file", + "file_write": "Write a file", + "file_edit": "Update a file", + "file_list": "List a directory", + "web_search": "Search the web", + "fetch": "Fetch a page", + "knowledge_search": "Search the library", + "knowledge_get": "Read a document", + "notes_search": "Search notes", + "notes_get": "Read a note", + "notes_create": "Write a note", + "notes_edit": "Change a note", + "notes_delete": "Delete a note", + "memory_add": "Remember something", + "memory_forget": "Forget something", + "skill_get": "Read a skill", + "skill_create": "Write a skill", + "skill_edit": "Change a skill", +} + +# Which argument is the thing being agreed to. Shown verbatim and escaped on the +# card: a summary that paraphrased it would be a card approving something other +# than what runs. +DETAIL_KEYS: dict[str, str] = { + "shell_run": "command", + "file_read": "path", + "file_write": "path", + "file_edit": "path", + "file_list": "path", + "fetch": "url", + "web_search": "query", + "knowledge_search": "query", + "notes_search": "query", +} + + +def _name_of(event: dict[str, Any] | str) -> str: + if isinstance(event, str): + return event + return str(event.get("name") or "") + + +def label_for(event: dict[str, Any] | str) -> str: + """What to call this tool in the transcript. + + The static table wins over anything stored on the event. See the module + docstring: rows already on disk carry the wrong label, and deferring to them + would leave every existing transcript naming a machine. + """ + name = _name_of(event) + if name in LABELS: + return LABELS[name] + if isinstance(event, dict): + stored = str(event.get("label") or "").strip() + if stored: + return stored + return name + + +def icon_for(event: dict[str, Any] | str) -> str: + """A symbol id for this event, never empty. + + Falls through the tool's own icon, then the event's `kind`, then the + generic one -- so a custom tool still gets a link and an MCP tool a server, + which is what the template used to decide for itself. + """ + name = _name_of(event) + if name in ICONS: + return ICONS[name] + kind = str(event.get("kind") or "") if isinstance(event, dict) else "" + if not kind and name == "web_search": + # Rows written before `kind` existed. The template made the same + # allowance for the same reason. + kind = "search" + return KIND_ICONS.get(kind, FALLBACK_ICON) + + +def describe(name: str, args: dict[str, Any]) -> tuple[str, str]: + """What an approval card says about one call: a title, and the detail.""" + title = ACTIONS.get(name) + key = DETAIL_KEYS.get(name) + if title is not None: + return title, str(args.get(key) or "") if key else "" + detail = ", ".join(f"{k}={v!r}" for k, v in list(args.items())[:4]) + return f"Use {label_for(name)}", detail[:400] + + +__all__ = [ + "ACTIONS", + "DETAIL_KEYS", + "FALLBACK_ICON", + "ICONS", + "KIND_ICONS", + "LABELS", + "describe", + "icon_for", + "label_for", +] diff --git a/src/lembas/web/templates/chat/_tool_activity.html b/src/lembas/web/templates/chat/_tool_activity.html index a98df11..e972cc2 100644 --- a/src/lembas/web/templates/chat/_tool_activity.html +++ b/src/lembas/web/templates/chat/_tool_activity.html @@ -19,23 +19,24 @@ `kind` says how to label the event. Rows written before it existed have none, so web_search reads as a search and everything else falls to the generic branch: an old notes_search event used to claim it had searched the web. + + What a tool is *called* is not decided here. `tool_label` and `tool_icon` are + Jinja globals over services/tool_labels.py, which is the one table the status + line and the approval card read too. It deliberately ignores an `event.label` + it recognises the name of: rows already on disk carry the SSH profile's name, + so a resolver that preferred the stored value would leave every existing + transcript saying "homeserver" where it means "Bash". #} {% for event in tool_events %} {% set kind = event.kind or ('search' if event.name == 'web_search' else 'tool') %}
- {% if kind == 'search' %} - {{ icon("globe", "icon--sm tool-activity__icon") }} - {% elif kind == 'custom' %} - {{ icon("link", "icon--sm tool-activity__icon") }} - {% elif kind == 'mcp' %} - {{ icon("server", "icon--sm tool-activity__icon") }} - {% else %} - {{ icon("sparkle", "icon--sm tool-activity__icon") }} - {% endif %} + {{ icon(tool_icon(event), "icon--sm tool-activity__icon") }} {% if kind == 'search' %} + {# Prose, not "Web search · mallorn". A search is the one thing here + common enough to be worth a sentence. #} {% if event.status == "error" %} Web search failed {% elif event.query %} @@ -44,7 +45,7 @@ Searched the web {% endif %} {% else %} - {% set label = event.label or event.name %} + {% set label = tool_label(event) %} {% if event.status == "error" %} {{ label }} failed {% else %} diff --git a/src/lembas/web/templates/partials/icons.html b/src/lembas/web/templates/partials/icons.html index ba61113..5abb003 100644 --- a/src/lembas/web/templates/partials/icons.html +++ b/src/lembas/web/templates/partials/icons.html @@ -195,6 +195,12 @@ + + + + + diff --git a/src/lembas/web/templating.py b/src/lembas/web/templating.py index e9ce371..c00ecbf 100644 --- a/src/lembas/web/templating.py +++ b/src/lembas/web/templating.py @@ -12,6 +12,7 @@ from lembas import __version__ from lembas.config import settings from lembas.db.models import User from lembas.services import metrics as metrics_service +from lembas.services import tool_labels from lembas.services.markdown import highlight_tokens from lembas.services.reasoning import format_duration @@ -51,6 +52,14 @@ templates.env.filters["stable_hue"] = stable_hue # every one of them would otherwise have to remember to pass it. templates.env.filters["tokens"] = highlight_tokens +# What a tool call is called and what it looks like. Globals rather than +# context values because a message bubble is rendered from four different +# handlers -- pages, post_message, regenerate and the SSE follower -- and every +# one of them would otherwise have to remember to pass them. That is the exact +# trap `audio_service.template_flags` fell into. +templates.env.globals["tool_label"] = tool_labels.label_for +templates.env.globals["tool_icon"] = tool_labels.icon_for + def resolve_theme(user: User | None) -> str: """Theme to render with on the server. diff --git a/tests/test_generation_tools.py b/tests/test_generation_tools.py index bc8638d..9e1ab8e 100644 --- a/tests/test_generation_tools.py +++ b/tests/test_generation_tools.py @@ -235,7 +235,9 @@ async def test_the_status_names_the_running_tool_and_is_cleared(db, user_id, mon generation = generation_service.Generation(chat_id=chat_id, message_id=message_id) await generation_service._run(generation) - assert seen == ["Running web_search…"] + # In words, from services/tool_labels.py -- the same table the transcript + # and the approval card read. It used to say "Running web_search…". + assert seen == ["Running Web search…"] assert generation.status == "", "and it is cleared once they are done" diff --git a/tests/test_tool_activity.py b/tests/test_tool_activity.py index 068aba6..5d3eb41 100644 --- a/tests/test_tool_activity.py +++ b/tests/test_tool_activity.py @@ -7,6 +7,12 @@ and everything in it is third-party text. from __future__ import annotations +import re +from pathlib import Path + +from lembas.services import tool_labels +from lembas.services import tools as tools_service +from lembas.services.agent import tools as agent_tools from lembas.web.templating import templates @@ -43,7 +49,7 @@ def test_a_library_tool_no_longer_claims_to_have_searched_the_web(): and "Searched the web for ".""" html = _render({"name": "notes_search", "query": "shopping", "status": "ok", "results": []}) assert "Searched the web" not in html - assert "notes_search" in html + assert "Notes searched" in html def test_a_custom_tool_is_named_and_its_host_shown(): @@ -122,3 +128,67 @@ def test_a_failure_shows_its_reason(): assert "tool-activity--error" in html assert "Weather failed" in html assert "HTTP 503" in html + + +# --- What a tool is called ----------------------------------------------------- +def test_a_stored_profile_name_no_longer_becomes_the_label(): + """The whole point of the inversion. + + Every agent event written before today carries `label` set to the SSH + profile's name, so the transcript said "homeserver · ls -la" and named the + machine rather than the thing that was done. Those rows are on disk and are + re-rendered on every page load, so the fix has to reach them -- which means + the static table wins over the stored value, not the other way round. + """ + html = _render( + { + "name": "shell_run", + "kind": "agent", + "label": "homeserver", + "query": "ls -la", + "detail": "homeserver:/srv/app", + "status": "ok", + "results": [], + } + ) + summary = html.split("")[0] + assert "Bash" in summary + assert "homeserver" not in summary + # It is still shown, in the body, where "where this ran" belongs. + assert "homeserver:/srv/app" in html + + +def test_a_custom_tools_own_label_still_wins(): + """The other half of the same rule. A row-backed tool's name is per row and + cannot be tabulated, so nothing in the table shadows it.""" + html = _render({"name": "weather", "kind": "custom", "label": "Weather", "results": []}) + assert "Weather" in html + + +def test_every_builtin_and_agent_tool_has_a_label_and_an_icon(): + """A property, not markup. A tool added without an entry renders its own + function name at somebody, which is the state this replaced.""" + names = [tool.name for tool in tools_service.REGISTRY.values()] + names += [tool.name for tool in agent_tools.tool_defs()] + # plan_submit is filtered out of tool_defs() outside Plan mode. + names.append("plan_submit") + missing = [name for name in names if name not in tool_labels.LABELS] + assert not missing, f"no label for {missing}" + missing = [name for name in names if name not in tool_labels.ICONS] + assert not missing, f"no icon for {missing}" + + +def test_every_icon_named_exists_in_the_sprite(): + """A typo'd symbol id renders an empty box and says nothing. This is the + only thing that catches it.""" + sprite = Path(tools_service.__file__).parents[1] / "web/templates/partials/icons.html" + available = set(re.findall(r'id="i-([a-z-]+)"', sprite.read_text())) + wanted = set(tool_labels.ICONS.values()) | set(tool_labels.KIND_ICONS.values()) + wanted.add(tool_labels.FALLBACK_ICON) + assert wanted <= available, f"not in the sprite: {sorted(wanted - available)}" + + +def test_an_unknown_tool_falls_back_to_its_name(): + assert tool_labels.label_for({"name": "mcp_thing"}) == "mcp_thing" + assert tool_labels.icon_for({"name": "mcp_thing", "kind": "mcp"}) == "server" + assert tool_labels.icon_for({"name": "whatever"}) == tool_labels.FALLBACK_ICON