diff --git a/src/lembas/api/admin_agents.py b/src/lembas/api/admin_agents.py index 3bda565..257cf0b 100644 --- a/src/lembas/api/admin_agents.py +++ b/src/lembas/api/admin_agents.py @@ -61,9 +61,10 @@ async def save_agents( default_timeout: int = Form(60), max_timeout: int = Form(600), max_output_bytes: int = Form(64 * 1024), - max_steps: int = Form(40), + max_steps: int = Form(200), max_wall_seconds: int = Form(900), max_total_output_bytes: int = Form(1024 * 1024), + max_completion_tokens: int = Form(200_000), approval_timeout: int = Form(900), allow_default: str = Form(""), deny_default: str = Form(""), @@ -75,6 +76,8 @@ async def save_agents( terminal_integration: bool = Form(False), index_enabled: bool = Form(False), index_chars: int = Form(2000), + instructions_enabled: bool = Form(False), + instructions_chars: int = Form(4000), ) -> Response: settings_store.update( db, @@ -86,9 +89,11 @@ async def save_agents( "default_timeout": min(max(default_timeout, 1), 3600), "max_timeout": min(max(max_timeout, 1), 3600), "max_output_bytes": min(max(max_output_bytes, 1024), 1024 * 1024), - "max_steps": min(max(max_steps, 1), 200), + "max_steps": min(max(max_steps, 1), 1000), "max_wall_seconds": min(max(max_wall_seconds, 30), 7200), "max_total_output_bytes": min(max(max_total_output_bytes, 4096), 8 * 1024 * 1024), + # Floor of 0, not 1: zero is how "no ceiling" is said. + "max_completion_tokens": min(max(max_completion_tokens, 0), 5_000_000), "approval_timeout": min(max(approval_timeout, 60), 3600), "allow_default": _lines(allow_default), "deny_default": _lines(deny_default), diff --git a/src/lembas/services/agent/policy.py b/src/lembas/services/agent/policy.py index cdf7477..e48b200 100644 --- a/src/lembas/services/agent/policy.py +++ b/src/lembas/services/agent/policy.py @@ -99,14 +99,26 @@ class Decision: class Limits: """What one agent reply may spend. - Three axes because they fail differently. Steps stop a loop; wall clock - stops a single slow command eating an afternoon; output stops a model - filling its own context with build logs and having no room left to answer. + Four axes because they fail differently. Wall clock stops a single slow + command eating an afternoon; `output_bytes` stops a model filling its own + context with build logs and having no room left to answer; and + `completion_tokens` stops one that keeps writing. + + `steps` is the odd one out. It is a **runaway backstop, not a working + budget** -- an agent reply is meant to run until the task is finished, and a + step count low enough to be the thing that ends it is a count that ends it + halfway. It was 40, which is a working budget, and it was reached. Anything + that wants a real ceiling should set `completion_tokens`, which measures + what a long reply actually costs. + + `completion_tokens` of 0 means no ceiling, the same convention `index_chars` + uses in the settings store. """ - steps: int = 40 + steps: int = 200 wall_seconds: float = 900.0 output_bytes: int = 1024 * 1024 + completion_tokens: int = 200_000 def subject(tool_name: str, command: str = "") -> str | None: diff --git a/src/lembas/services/agent/session.py b/src/lembas/services/agent/session.py index 14693f0..6367679 100644 --- a/src/lembas/services/agent/session.py +++ b/src/lembas/services/agent/session.py @@ -141,9 +141,12 @@ def resolve(db: DBSession, chat: Chat, user: User | None) -> AgentContext | None allow=tuple(values.get("allow_default") or ()), deny=tuple(values.get("deny_default") or ()), limits=Limits( - steps=int(values.get("max_steps") or 40), + steps=int(values.get("max_steps") or 200), wall_seconds=float(values.get("max_wall_seconds") or 900), output_bytes=int(values.get("max_total_output_bytes") or 1024 * 1024), + # `or 0` would turn a deliberate 0 into the default, and 0 is how an + # administrator says "no ceiling". `agents()` has already clamped it. + completion_tokens=int(values.get("max_completion_tokens", 200_000) or 0), ), timeout=float(values.get("default_timeout") or 60), max_timeout=float(values.get("max_timeout") or 600), diff --git a/src/lembas/services/generation.py b/src/lembas/services/generation.py index 06c01cc..ca54680 100644 --- a/src/lembas/services/generation.py +++ b/src/lembas/services/generation.py @@ -379,6 +379,10 @@ async def _run(generation: Generation) -> None: if generation.output_bytes > limits.output_bytes: _gave_up(generation, "with too much output to read") break + written = _written(generation) + if limits.completion_tokens and written > limits.completion_tokens: + _gave_up(generation, f"after writing about {written:,} tokens") + break accumulator = tools_service.ToolCallAccumulator() # Text the model produced in *this* round, needed separately from # generation.content when echoing the assistant turn back. @@ -446,13 +450,13 @@ async def _run(generation: Generation) -> None: # chat allowed forty steps stopped after three and said it had # taken forty. Two numbers, one of them wrong, in code whose # whole job is to say what happened. + howmany = "one round" if budget == 1 else f"{budget} rounds" generation.tool_events.append( { "name": calls[0]["name"], "status": "error", "error": ( - f"Stopped after {budget} rounds of tool calls " - f"without an answer." + f"Stopped after {howmany} of tool calls without an answer." ), } ) diff --git a/src/lembas/services/harness.py b/src/lembas/services/harness.py index c5e26b2..dcfb3dc 100644 --- a/src/lembas/services/harness.py +++ b/src/lembas/services/harness.py @@ -125,6 +125,11 @@ def context_variables( "user_name": (user.name or "") if user is not None else "", "model_name": "", "max_rounds": str(tools_service.MAX_ROUNDS), + # Not rendered anywhere. It is the gate on `core.rounds`: an ordinary + # chat gets one round and is told to ask for everything at once, an + # agent chat is told to keep going, and those are different sentences + # rather than the same sentence with a different number in it. + "round_budget": str(tools_service.MAX_ROUNDS), "memory_limit": str(memories_service.MAX_MEMORY_CHARS), "tool_names": _tool_names(offered), "memories": memories_service.block(db, user) if "memory" in families else "", @@ -181,6 +186,10 @@ def _agent_values(db: DBSession, chat, user) -> dict[str, str]: "agent_mode": policy.MODE_GUIDANCE.get(context.mode, ""), "agent_rewound": rewound, "max_rounds": str(context.limits.steps), + # Blanked, which is what makes `core.rounds` vanish here: `steps` is a + # runaway backstop and telling a model it has a budget of two hundred + # invites it to ration one. + "round_budget": "", "project_files": _project_files(db, chat, context, settings_store, index_service), } diff --git a/src/lembas/services/prompts.py b/src/lembas/services/prompts.py index cd28c3a..7538de0 100644 --- a/src/lembas/services/prompts.py +++ b/src/lembas/services/prompts.py @@ -128,6 +128,13 @@ VARIABLES: tuple[Variable, ...] = ( Variable("user_name", "User's name", "The name of the person in the conversation."), Variable("model_name", "Model", "The display name of the model answering."), Variable("max_rounds", "Tool rounds", "How many rounds of tool calls one reply may take."), + Variable( + "round_budget", + "Round budget applies", + "Set in an ordinary chat and blank in an agent chat. Nothing renders it; " + "it exists so a fragment can say `requires=('round_budget',)` and appear " + "for one and not the other.", + ), Variable( "memory_limit", "Memory length", @@ -572,19 +579,64 @@ BUILTIN: tuple[Fragment, ...] = ( "permission first." ), ), + Fragment( + key="core.tool_list", + label="What you have", + group=GROUP_CORE, + order=105, + when_tools=True, + variables=("tool_names",), + requires=("tool_names",), + hint="The names of the tools offered on THIS request, which is not the " + "same as the tools that exist -- a chat can narrow them, a model's " + "capabilities can, a permission can. A model that has to discover its " + "own list by calling something and being told it does not exist spends " + "a round finding out, and in an ordinary chat that round is the whole " + "reply. It is also what stops a model hunting for a skill when there " + "are none.", + default=( + "The tools you have on this request are: {{tool_names}}. That is the whole " + "list. Anything not named there does not exist here — calling it costs a " + "round and returns nothing." + ), + ), Fragment( key="core.rounds", label="The round budget", group=GROUP_CORE, order=110, when_tools=True, - variables=("max_rounds",), - hint="A model that plans six searches gets cut off after three. Better it " - "knows the budget than discovers it.", + requires=("round_budget",), + hint="An ordinary chat only. It gets ONE round of tool calls, and the " + "thing worth saying about one round is 'ask for everything at once' — " + "which is different in kind from what is true of an agent chat's two " + "hundred, not a different number in the same sentence. So this is " + "gated on `round_budget`, which `_agent_values` blanks, and the agent " + "case is its own fragment below.", default=( - "You get at most {{max_rounds}} rounds of tool calls before you have to " - "answer with what you have. Several tools can be called in one round. Plan " - "within that budget: two careful searches beat six that run out halfway." + "You get one round of tool calls, and then you have to answer with what " + "came back. Ask for everything you need at once — several tools can be " + "called in the same round. If what comes back is not enough, say what you " + "would look up next rather than answering as though it were." + ), + ), + Fragment( + key="core.keep_working", + label="Working until it is done", + group=GROUP_CORE, + order=111, + families=("agent",), + hint="An agent chat only, and the counterpart to the round budget above. " + "A model told it has a budget rations it and stops early to report " + "progress; the step count here is a runaway backstop, not an " + "allowance, and saying so is what makes a long piece of work run.", + default=( + "Keep working until the task is actually done. You are not rationing a " + "round budget: call tools as many times as the work needs, one step " + "informing the next. What ends a reply is finishing it, being stopped, or " + "running past the time and output an administrator allowed — and if that " + "happens you are told so and can be asked to carry on. Do not stop halfway " + "to report progress and wait to be told to continue." ), ), Fragment( diff --git a/src/lembas/services/settings_store.py b/src/lembas/services/settings_store.py index 582ef19..739e004 100644 --- a/src/lembas/services/settings_store.py +++ b/src/lembas/services/settings_store.py @@ -66,9 +66,19 @@ def _agents_defaults() -> dict[str, Any]: "max_timeout": 600, "max_output_bytes": 64 * 1024, # Per reply. See services/agent/policy.py:Limits. - "max_steps": 40, + # + # `max_steps` is a runaway backstop rather than a working budget: an + # agent reply is meant to run until the task is done, and a step count + # low enough to be the thing that stops it is a count that stops it + # halfway. What actually bounds a long reply is the wall clock and + # `max_completion_tokens`. + "max_steps": 200, "max_wall_seconds": 900, "max_total_output_bytes": 1024 * 1024, + # How much the model may *write* in one reply, across every round. + # Zero means no ceiling, which is a thing somebody may want and has no + # other way of being said -- the same convention as `index_chars`. + "max_completion_tokens": 200_000, # How long a reply waits for someone to answer. Clamped on read: a zero # here would park a background task forever. "approval_timeout": 900, @@ -261,4 +271,11 @@ def agents(db: DBSession) -> dict[str, Any]: # directory for the file picker, but put none of it in the prompt", which # is a reasonable thing to want and has no other way of being said. values["index_chars"] = min(max(int(values.get("index_chars") or 0), 0), 20_000) + values["instructions_chars"] = min( + max(int(values.get("instructions_chars") or 0), 0), 20_000 + ) + # Zero is meaningful here too: no ceiling on what one reply may write. + values["max_completion_tokens"] = min( + max(int(values.get("max_completion_tokens") or 0), 0), 5_000_000 + ) return values diff --git a/src/lembas/services/tools.py b/src/lembas/services/tools.py index 2d83907..08c7841 100644 --- a/src/lembas/services/tools.py +++ b/src/lembas/services/tools.py @@ -45,16 +45,31 @@ from lembas.services.search.base import SearchError log = logging.getLogger(__name__) -# How many times a model may call tools before it has to answer with words. -# Not a safety limit so much as a termination one: a small model that has -# decided searching is the answer will otherwise search until the context runs -# out, and each round costs a full request. -MAX_ROUNDS = 3 +# How many times a model may call tools before it has to answer with words, in +# an ORDINARY chat. An agent chat is sized by `agent/policy.py:Limits.steps` +# instead, which is two orders of magnitude larger, because an agent reply is +# meant to run until the work is done. +# +# One, deliberately. A plain conversation asking a question is one round of +# looking things up and then an answer; the rounds after that were a small model +# that had decided searching was the answer searching until the context ran out, +# at a full request each. Several tools can still be called *within* that round, +# which is the thing worth telling the model -- see `core.rounds`. +# +# The trade is real and worth naming: a chat can no longer search and then read +# one of the results, because reading is a second round. That is what an agent +# chat is for. +MAX_ROUNDS = 1 # Tool families, matching the per-model capability flags and the permission # keys. The three names differ by prefix only, which is deliberate: adding a # family means adding one entry here and one permission. FAMILY_SEARCH = "web_search" +# Reading one page, given its address. Its own family rather than part of +# `web_search`: an administrator may reasonably want a model that can look +# things up but not follow an arbitrary URL it read somewhere, and the SSRF +# surface is entirely on this side. +FAMILY_FETCH = "fetch" FAMILY_KNOWLEDGE = "knowledge" FAMILY_NOTES = "notes" FAMILY_MEMORY = "memory" diff --git a/src/lembas/web/templates/admin/agents.html b/src/lembas/web/templates/admin/agents.html index 1066ead..67348c0 100644 --- a/src/lembas/web/templates/admin/agents.html +++ b/src/lembas/web/templates/admin/agents.html @@ -127,15 +127,22 @@

What one reply may spend

- Three separate bounds, because they fail differently: steps stop a loop, - the clock stops one slow command eating an afternoon, and output stops a - model filling its own context with build logs and having no room to answer. + Four separate bounds, because they fail differently: the clock stops one + slow command eating an afternoon, tool output stops a model filling its own + context with build logs and having no room to answer, written tokens stop + one that keeps going, and the step count is a backstop against a runaway.

- - + + +

+ In tokens, across every round of one reply. This is the bound that + normally ends a long piece of work. Zero means no ceiling. +

@@ -148,6 +155,16 @@
+
+ + +

+ A backstop, not a working budget. An agent reply is meant to run until + the task is done, so a number low enough to be what stops it is a number + that stops it halfway. Use the token ceiling above for a real limit. +

+
diff --git a/tests/test_agent_profiles.py b/tests/test_agent_profiles.py index 0c1234a..63a42bf 100644 --- a/tests/test_agent_profiles.py +++ b/tests/test_agent_profiles.py @@ -391,9 +391,10 @@ def test_the_numbers_are_clamped(client: TestClient, db, registered): "default_timeout": "0", "max_timeout": "99999", "max_output_bytes": "1", - "max_steps": "9999", + "max_steps": "99999", "max_wall_seconds": "1", "max_total_output_bytes": "1", + "max_completion_tokens": "0", "approval_timeout": "0", "allow_default": "", "deny_default": "", @@ -403,8 +404,11 @@ def test_the_numbers_are_clamped(client: TestClient, db, registered): values = settings_store.agents(db) assert values["default_timeout"] == 1 assert values["max_timeout"] == 3600 - assert values["max_steps"] == 200 + assert values["max_steps"] == 1000 assert values["approval_timeout"] == 60, "a zero would park a task forever" + # Not clamped up to a minimum: zero is how "no ceiling on what a reply may + # write" is said, exactly as it is for index_chars. + assert values["max_completion_tokens"] == 0 def test_an_unticked_checkbox_turns_it_off(client: TestClient, db, registered): diff --git a/tests/test_agent_tools.py b/tests/test_agent_tools.py index 76f8592..bffff62 100644 --- a/tests/test_agent_tools.py +++ b/tests/test_agent_tools.py @@ -741,6 +741,88 @@ async def test_an_agent_chat_gets_the_rounds_it_was_promised(db, user_id, machin assert "after 5 rounds" in generation.tool_events[-1]["error"] +async def test_a_reply_stops_when_it_has_written_too_much(db, user_id, machine, monkeypatch): + """The bound that is meant to end a long piece of work. + + Steps are a runaway backstop now (200), so something has to say when enough + has been written. Asserted on the loop, not on the wording: the count of + requests must be far short of the step budget. + """ + settings_store.update( + db, {"max_steps": 200, "max_completion_tokens": 40}, key=settings_store.AGENTS + ) + chat, _profile = _setup(db, user_id, machine, mode=policy.MODE_AUTO) + message_id = _pending_reply(db, chat) + + payloads: list[dict] = [] + monkeypatch.setattr( + generation_service, + "stream_chat", + _stub_stream( + [[_text("x" * 400), _chunk("file_list", '{"path": "."}')]], + payloads, + ), + ) + + async def _no_title(*_args, **_kwargs): + return "" + + monkeypatch.setattr("lembas.services.chat.generate_title", _no_title) + + generation = generation_service.Generation(chat_id=chat.id, message_id=message_id) + await generation_service._run(generation) + + assert len(payloads) < 5, "it should have stopped long before the step backstop" + assert "tokens" in generation.tool_events[-1]["error"] + + +async def test_the_token_ceiling_fires_on_an_endpoint_that_reports_no_usage( + db, user_id, machine, monkeypatch +): + """The half that would otherwise be silently broken. + + `generation.completion_tokens` is only populated when the endpoint sends a + usage block, and llama.cpp, Ollama and friends never do -- the fallback + estimate is computed once, in `_run`'s `finally:`, long after the loop that + needs it. A ceiling reading only the reported figure would work on OpenAI + and do nothing at all everywhere else. The stub above sends no usage, so + this asserts the estimate path directly. + """ + settings_store.update( + db, {"max_steps": 200, "max_completion_tokens": 40}, key=settings_store.AGENTS + ) + chat, _profile = _setup(db, user_id, machine, mode=policy.MODE_AUTO) + message_id = _pending_reply(db, chat) + + payloads: list[dict] = [] + monkeypatch.setattr( + generation_service, + "stream_chat", + _stub_stream([[_text("y" * 400), _chunk("file_list", '{"path": "."}')]], payloads), + ) + + async def _no_title(*_args, **_kwargs): + return "" + + monkeypatch.setattr("lembas.services.chat.generate_title", _no_title) + + generation = generation_service.Generation(chat_id=chat.id, message_id=message_id) + await generation_service._run(generation) + + assert not any("usage" in str(p) for p in payloads), "the stub reports no usage" + assert "tokens" in generation.tool_events[-1]["error"] + + +async def test_a_zero_ceiling_means_no_ceiling(db, user_id, machine, monkeypatch): + """Zero is how an administrator says "no limit", the same as index_chars. + Read with `or 0` on the wrong side it would silently become 200_000.""" + settings_store.update(db, {"max_completion_tokens": 0}, key=settings_store.AGENTS) + chat, _profile = _setup(db, user_id, machine, mode=policy.MODE_AUTO) + user = db.get(User, user_id) + context = session.resolve(db, chat, user) + assert context.limits.completion_tokens == 0 + + # --- Interjecting while it works -------------------------------------------------- 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_generation_tools.py b/tests/test_generation_tools.py index 9e1ab8e..7c14153 100644 --- a/tests/test_generation_tools.py +++ b/tests/test_generation_tools.py @@ -163,10 +163,59 @@ async def test_a_model_that_only_ever_calls_tools_is_stopped(db, user_id, monkey generation = generation_service.Generation(chat_id=chat_id, message_id=message_id) await generation_service._run(generation) - assert len(payloads) == tools_service.MAX_ROUNDS + 1 + # One round that may call tools, then one that has to answer with words. + # Spelled out rather than derived from the constant: a test that reads + # MAX_ROUNDS passes whatever MAX_ROUNDS becomes, which is exactly the + # assertion nobody wanted. + assert tools_service.MAX_ROUNDS == 1 + assert len(payloads) == 2 # Recorded rather than silently dropped: an answer that stops here has to # be explicable. assert generation.tool_events[-1]["status"] == "error" + assert "one round" in generation.tool_events[-1]["error"] + + +async def test_a_prompt_queued_during_a_one_round_reply_waits_for_its_own( + db, user_id, monkeypatch +): + """`_inject` only takes a prompt in while there is a round left to answer in, + and with one round there never is -- so a queued message is not swallowed + into a reply that then has no chance to address it. It waits for `_drain`, + which always gives it a reply of its own. + + No code change went with this; it falls out of the guard. The test is here + because "it happens to work" and "it is meant to work" look the same until + somebody changes the guard. + """ + from lembas.db.models import Message + from lembas.services import chat as chat_service + + settings_store.update(db, {"enabled": True}, key=settings_store.SEARCH) + chat_id, message_id = _chat_with_tools(db, user_id) + chat = db.get(Chat, chat_id) + queued = chat_service.create_message(db, chat, "user", "actually, do it the other way", + queued=True) + queued_id = queued.id + + monkeypatch.setattr("lembas.services.search.run", _empty_search) + monkeypatch.setattr( + generation_service, + "stream_chat", + _stub_stream( + [[_tool_call_chunk("web_search", '{"query": "x"}')], [_text_chunk("Done.")]], + [], + ), + ) + monkeypatch.setattr("lembas.services.chat.generate_title", _never_called_title) + + generation = generation_service.Generation(chat_id=chat_id, message_id=message_id) + await generation_service._run(generation) + + # The row, not the payload: it was handed to a fresh reply by `_drain`, + # which is what clears `queued`. + db.expire_all() + assert db.get(Message, queued_id).queued is False + assert generation.drained is True async def test_tool_activity_is_stored_with_the_message(db, user_id, monkeypatch): diff --git a/tests/test_harness.py b/tests/test_harness.py index c03875a..036ff38 100644 --- a/tests/test_harness.py +++ b/tests/test_harness.py @@ -357,3 +357,26 @@ def test_a_plain_chat_is_told_nothing_about_files(db, owner): db.commit() assert "Files in" not in harness.compose(db, owner, _tools("web_search"), chat=chat) + + +# --- One round, or as many as it takes ---------------------------------------- +def test_a_plain_chat_is_told_it_has_one_round(db, owner): + """And is told to ask for everything at once, which is the advice that + matters when there is only one.""" + text = harness.compose(db, owner, _tools("web_search")) + + assert "one round of tool calls" in text + assert "Keep working until the task is actually done" not in text + + +def test_an_agent_chat_is_told_to_keep_going_instead(db, owner): + """The two cannot be one fragment with a number in it. A model told it has + a budget rations it; the step count is a runaway backstop, and rationing + against it is exactly the behaviour that stops a long piece of work + halfway.""" + chat, _profile = _agent_chat(db, owner) + + text = harness.compose(db, owner, _agent_tools(db), chat=chat) + + assert "Keep working until the task is actually done" in text + assert "one round of tool calls" not in text