Say what a tool did, not where it ran
An agent event set its label to the SSH profile's name, so the transcript read "homeserver · ls -la" -- naming the machine rather than the thing that was done. Built-in tools set no label at all and fell back to the function name, so a saved memory read "memory_add". The status line said "Running shell_run…" and the approval card had its own hand-written wording. Four places, four answers, nothing checking that any of them agreed. services/tool_labels.py is the one table all of them read now. Bash, Read, Write, List, Web search, Memory saved; an icon each, instead of everything being the sparkle. The precedence is inverted on purpose. Tool events are persisted in Message.tool_calls_json, so every agent row already on disk carries the profile name -- a resolver that preferred the stored value would fix nothing for any transcript that already exists. So a name the table knows resolves from the table, and a name it does not -- a custom HTTP tool, an MCP tool, whose labels are per row and cannot be tabulated -- keeps its own. One rule, both cases correct. The machine moves to `detail`, where "where this ran" belongs. tool_label and tool_icon are Jinja globals because a message bubble is rendered from four handlers, and a fifth thing each of them must remember to pass is a fifth thing one of them will forget. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -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,
|
||||
}
|
||||
|
||||
@@ -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]:
|
||||
|
||||
@@ -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",
|
||||
]
|
||||
@@ -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') %}
|
||||
<details class="tool-activity {{ 'tool-activity--error' if event.status == 'error' }}">
|
||||
<summary class="tool-activity__summary">
|
||||
{% 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") }}
|
||||
|
||||
<span class="tool-activity__label">
|
||||
{% 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 %}
|
||||
|
||||
@@ -195,6 +195,12 @@
|
||||
<path d="M13.5 3.5V9H19M8.5 13h7M8.5 16.5h7"/>
|
||||
</symbol>
|
||||
|
||||
<!-- Changing part of a file. A plus over a minus, which is what a diff looks
|
||||
like everywhere else; the pencil is already taken by writing one whole. -->
|
||||
<symbol id="i-diff" viewBox="0 0 24 24">
|
||||
<path d="M12 3.5v7M8.5 7h7M8.5 17h7"/>
|
||||
</symbol>
|
||||
|
||||
<!-- A drag handle. Dots rather than lines: lines at this size read as a
|
||||
hamburger, which means something else entirely. -->
|
||||
<symbol id="i-grip" viewBox="0 0 24 24">
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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"
|
||||
|
||||
|
||||
|
||||
@@ -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 <the note title>"."""
|
||||
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("</summary>")[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
|
||||
|
||||
Reference in New Issue
Block a user