diff --git a/src/lembas/services/generation.py b/src/lembas/services/generation.py index 30bb954..173f61c 100644 --- a/src/lembas/services/generation.py +++ b/src/lembas/services/generation.py @@ -450,7 +450,14 @@ async def _run(generation: Generation) -> None: # tokens and the clock instead. budget = limits.steps if limits else (chat_rounds or MAX_TOOL_ROUNDS) - for round_number in range(budget + 1): + # Set once a budget has run out, holding the last round open with the + # tools withdrawn so the reply ends in an answer rather than in silence. + # See `_wrap_up`. `budget + 2` rather than `+ 1` is that extra round: + # the iteration at `budget` is where the overrun is noticed, and the one + # after it is where the model gets to say what it found. + wrapping_up = False + + for round_number in range(budget + 2): generation.rounds = round_number + 1 # Recomputed every round, against once before the loop. The request @@ -467,6 +474,11 @@ async def _run(generation: Generation) -> None: # 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. + # The one budget that still stops dead rather than asking for a final + # answer. Every other one can afford one more request; this one is + # the finding that there is no room for a request, and a wrap-up + # round would be the same overflow with an upstream error instead of + # an explanation. if round_number and _too_big(generation): _gave_up(generation, "with no room left in the context window") break @@ -476,18 +488,20 @@ async def _run(generation: Generation) -> None: # Stop already covers the mid-stream case. Time spent waiting for a # person is subtracted -- somebody who thinks for ten minutes about # one command should not thereby spend the whole allowance. - if limits is not None and round_number: + if limits is not None and round_number and not wrapping_up: spent = (time.monotonic() - started) - generation.waited + ran_out = "" if spent > limits.wall_seconds: - _gave_up(generation, f"after {spent / 60:.0f} minutes") - break - 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 + ran_out = f"after {spent / 60:.0f} minutes" + elif generation.output_bytes > limits.output_bytes: + ran_out = "with too much output to read" + else: + written = _written(generation) + if limits.completion_tokens and written > limits.completion_tokens: + ran_out = f"after writing about {written:,} tokens" + if ran_out: + offered, payload = _wrap_up(generation, ran_out, payload) + wrapping_up = True accumulator = tools_service.ToolCallAccumulator() # Text the model produced in *this* round, needed separately from # generation.content when echoing the assistant turn back. @@ -582,29 +596,37 @@ async def _run(generation: Generation) -> None: # not. The count is of *consecutive* stops. generation.nudges = 0 - if round_number == budget: - # Out of rounds with the model still asking for tools. Recorded - # rather than silently dropped: an answer that stops here needs - # to be explicable. + if round_number >= budget: + # Out of rounds with the model still asking for tools. # - # `budget`, not `MAX_ROUNDS`. The loop is sized by the budget on - # the line above and the message below has always reported it, - # but the comparison was against the global 3 -- so an agent - # 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. + # The tools are withdrawn and it is asked once more, rather than + # the reply simply ending here. A model that goes straight to + # tool calls has written no prose at all by this point, so + # breaking produced an empty bubble with an error line under it + # -- somebody watching a good piece of research get to its sixth + # search saw the whole thing thrown away. What it has gathered is + # in the transcript either way; one more request turns it into an + # answer. + # + # `budget`, not `MAX_ROUNDS`. The loop is sized by the budget + # above and the message below has always reported it, but the + # comparison was against the global 3 -- so an agent 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 {howmany} of tool calls without an answer." - ), - } + offered, payload = _wrap_up( + generation, + f"after {howmany} of tool calls", + payload, + name=calls[0]["name"], ) - generation.touch() - break + if wrapping_up: + # Already asked, and it called a tool anyway -- which it + # cannot do, since none were offered. A backstop, not a path. + break + wrapping_up = True + continue # Parsed once, here, and shared by everything below: the approval # card, `policy.decide`, and the runner. See `_arguments_for`. @@ -930,6 +952,40 @@ def _gave_up(generation, why: str) -> None: generation.touch() +def _wrap_up(generation, why: str, payload: dict, *, name: str = "budget") -> tuple[list, dict]: + """A budget has run out. Withdraw the tools and ask for an answer. + + Returns the empty tool list and the payload without its `tools` array, so + the next request is one the model can only answer. + + Every budget used to end the reply where it was noticed, which is fine for a + model that narrates as it works and produces nothing at all for one that goes + straight to tool calls: an empty bubble with a red line under it, and a good + piece of research thrown away at its sixth search. What it has gathered is + already in the transcript, so one more request without tools turns it into + an answer. That is the same move `plan_submit` makes -- a turn should not end + mid-sentence -- and it is why the loop runs to `budget + 2`. + + The event still goes in the transcript. The reader has to be able to tell an + answer the model chose to give from one it gave because it ran out of room, + and those read identically otherwise. + """ + generation.tool_events.append( + { + "name": name, + "kind": "agent", + "status": "error", + "results": [], + "error": ( + f"Stopped {why}. What follows is an answer from what had been " + "gathered by then; ask again to carry on." + ), + } + ) + generation.touch() + return [], {key: value for key, value in payload.items() if key != "tools"} + + def _nudge( generation: Generation, context, diff --git a/src/lembas/services/harness.py b/src/lembas/services/harness.py index 9469cb1..17f9b32 100644 --- a/src/lembas/services/harness.py +++ b/src/lembas/services/harness.py @@ -149,6 +149,11 @@ def context_variables( # same sentence with a different number in it. Blank when there is no # ceiling at all, so the fragment vanishes rather than promising zero. "round_budget": str(settings_store.chat_rounds(db) or ""), + # The complement, and the gate on `core.keep_working`. Exactly one of + # the two is ever set: a model told it has a budget rations it and stops + # early to report progress, and one told to keep going does the work. + # Not rendered anywhere either. + "unbounded": "" if settings_store.chat_rounds(db) else "yes", "memory_limit": str(memories_service.MAX_MEMORY_CHARS), "tool_names": _tool_names(offered), "memories": memories_service.block(db, user) if "memory" in families else "", @@ -219,8 +224,11 @@ def _agent_values(db: DBSession, chat, user) -> dict[str, str]: "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. + # invites it to ration one. `unbounded` is its complement and is what + # `core.keep_working` is gated on, so an agent chat always gets the + # keep-going half whatever the instance setting says. "round_budget": "", + "unbounded": "yes", "project_files": _project_files(db, chat, context, settings_store, index_service), # Already resolved on the context, from one primary-key lookup in # `agent_session.resolve`. A plan the model cannot see is a plan it diff --git a/src/lembas/services/prompts.py b/src/lembas/services/prompts.py index e05b5af..6c84eec 100644 --- a/src/lembas/services/prompts.py +++ b/src/lembas/services/prompts.py @@ -131,9 +131,20 @@ VARIABLES: tuple[Variable, ...] = ( 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.", + "Set when an administrator has put a ceiling on an ordinary chat's tool " + "rounds, and blank otherwise — including in every agent chat. Nothing " + "renders it; it exists so a fragment can say " + "`requires=('round_budget',)` and appear only where there is a budget " + "worth planning within.", + ), + Variable( + "unbounded", + "No round budget", + "The exact complement of the one above: set whenever `round_budget` is " + "blank. Nothing renders this either. Two gates rather than one because " + "what is worth telling a model with a budget and what is worth telling " + "one that should work until the job is done are different sentences, " + "not the same sentence with a different number in it.", ), Variable( "memory_limit", @@ -656,18 +667,22 @@ BUILTIN: tuple[Fragment, ...] = ( 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.", + when_tools=True, + requires=("unbounded",), + hint="The counterpart to the round budget above, and exactly one of the " + "two ever appears: `unbounded` is set precisely when `round_budget` is " + "not. A model told it has a budget rations it and stops early to report " + "progress; where the number is a runaway backstop rather than an " + "allowance, saying so is what makes a long piece of work run. An agent " + "chat always gets this one; an ordinary chat gets it whenever an " + "administrator has set no ceiling, which is now the default.", 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." + "running out of room — and if you run out you are told so, asked for an " + "answer from what you have, and can be asked to carry on afterwards. 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 643888c..3813fac 100644 --- a/src/lembas/services/settings_store.py +++ b/src/lembas/services/settings_store.py @@ -25,7 +25,7 @@ AUDIO = "audio" # Used when nothing is stored. `services/tools.py:MAX_ROUNDS` is the same number # and exists for callers with no session -- this module is where the setting is # read, and the two are asserted equal by a test so they cannot drift. -DEFAULT_CHAT_ROUNDS = 5 +DEFAULT_CHAT_ROUNDS = 0 SEARCH = "search" PROMPTS = "prompts" AGENTS = "agents" @@ -53,11 +53,21 @@ def _general_defaults() -> dict[str, Any]: # model saying it has what it needs. This only catches the case where it # never says so. # - # Five rather than one because several built-in tools are two-step pairs - # -- knowledge_get and notes_get read a document "by the id a search - # returned" -- so a ceiling of one makes the second half unreachable and - # the library searchable but not readable. Zero means no ceiling. - "max_chat_rounds": 5, + # Zero, meaning no ceiling, and the loop falls back to MAX_TOOL_ROUNDS + # as a runaway backstop -- the same shape `Limits.steps` has for an agent + # chat. It was 1, then 5, and both were the same mistake at different + # scales: a number low enough to be reached by ordinary work is not a + # ceiling, it is a schedule, and it overrides the model's judgement on + # every turn rather than catching a runaway. Five was reached by a small + # local model doing a genuinely good piece of research -- six searches, + # each one informed by the last -- and the reply ended there. + # + # What actually bounds an ordinary chat is the context window + # (`CONTEXT_HEADROOM`), which is a real limit rather than a guess at how + # much looking-up a question deserves. An administrator who wants a + # ceiling can still set one, and `core.rounds` then tells the model it + # has one; with none, `core.keep_working` tells it to work until done. + "max_chat_rounds": 0, } diff --git a/src/lembas/services/tools.py b/src/lembas/services/tools.py index 3fb9962..97aee48 100644 --- a/src/lembas/services/tools.py +++ b/src/lembas/services/tools.py @@ -58,15 +58,21 @@ log = logging.getLogger(__name__) # searching is the answer, searching until the context runs out at a full # request each. # -# It was briefly 1, which is low enough to stop being a ceiling and start being -# a schedule: it overrode the model's judgement on every turn rather than -# catching a runaway. Worse, several built-ins are two-step pairs -- -# `knowledge_get` and `notes_get` read a document "by the id a search returned" -# -- so one round left the library searchable and not readable. +# It was 1, then 5, and now 0 meaning no ceiling at all. Both numbers were the +# same mistake at different scales: low enough to be reached by ordinary work is +# low enough to be a schedule rather than a ceiling, overriding the model's +# judgement on every turn instead of catching a runaway. One left the library +# searchable and not readable, since `knowledge_get` and `notes_get` read a +# document "by the id a search returned". Five ended a piece of research at its +# sixth search. +# +# What bounds an ordinary chat now is the context window, and the loop falls +# back to `generation.MAX_TOOL_ROUNDS` as a runaway backstop -- the shape +# `Limits.steps` already had for an agent chat. # # `settings_store.chat_rounds()` is what the loop and the harness read; this is # the fallback for callers with no session, and a test pins the two together. -MAX_ROUNDS = 5 +MAX_ROUNDS = 0 # Tool families, matching the per-model capability flags and the permission # keys. The three names differ by prefix only, which is deliberate: adding a diff --git a/tests/test_agent_tools.py b/tests/test_agent_tools.py index 2eab6dc..a3695e8 100644 --- a/tests/test_agent_tools.py +++ b/tests/test_agent_tools.py @@ -1037,8 +1037,10 @@ async def test_an_agent_chat_gets_the_rounds_it_was_promised(db, user_id, machin generation = generation_service.Generation(chat_id=chat.id, message_id=assistant.id) await generation_service._run(generation) - # Five rounds that may call tools, then the one that gives up. - assert len(payloads) == 6 + # Five rounds that may call tools, the one that notices, and the one asked + # for an answer with the tools withdrawn. + assert len(payloads) == 7 + assert "tools" not in payloads[-1] assert "after 5 rounds" in generation.tool_events[-1]["error"] diff --git a/tests/test_generation_tools.py b/tests/test_generation_tools.py index 696126e..7140297 100644 --- a/tests/test_generation_tools.py +++ b/tests/test_generation_tools.py @@ -167,14 +167,106 @@ 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) - # The ceiling, plus the round that has to answer with words. - assert len(payloads) == 4 + # Three rounds that may call tools, the one that notices the ceiling, and + # the one asked for an answer with the tools withdrawn. + assert len(payloads) == 5 + # And the last request carried no tools at all, which is what makes it a + # round the model can only answer. + assert "tools" not in payloads[-1] # Recorded rather than silently dropped: an answer that stops here has to # be explicable. assert generation.tool_events[-1]["status"] == "error" assert "3 rounds" in generation.tool_events[-1]["error"] +async def test_running_out_of_rounds_still_produces_an_answer(db, user_id, monkeypatch): + """The bug this exists for. + + A model that goes straight to tool calls has written no prose at all by the + time a budget runs out, so ending the reply there produced an empty bubble + with an error line under it -- a good piece of research, six searches deep, + thrown away. The tools are withdrawn and it is asked once more instead: what + it gathered is in the transcript either way, and one request turns it into + an answer. + """ + settings_store.update(db, {"enabled": True}, key=settings_store.SEARCH) + chat_id, message_id = _chat_with_tools(db, user_id) + monkeypatch.setattr("lembas.services.search.run", _empty_search) + settings_store.update(db, {"max_chat_rounds": 2}) + + payloads: list[dict] = [] + + async def stream_chat(_endpoint, payload): + payloads.append(payload) + # Exactly what a real model does: call tools while it has them, and + # answer when it has none. + if payload.get("tools"): + yield _tool_call_chunk("web_search", '{"query": "x"}') + else: + yield {"choices": [{"delta": {"content": "Here is what I found."}}]} + + monkeypatch.setattr(generation_service, "stream_chat", stream_chat) + 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) + + assert generation.text == "Here is what I found." + assert not generation.error + # And the reader can still tell this apart from an answer the model chose + # to give, which is what the event is for. + assert generation.tool_events[-1]["status"] == "error" + assert "2 rounds" in generation.tool_events[-1]["error"] + + +async def test_no_ceiling_by_default(db, user_id): + """A number low enough to be reached by ordinary work is a schedule, not a + ceiling. What bounds an ordinary chat is the context window.""" + assert settings_store.chat_rounds(db) == 0 + + +async def test_without_a_ceiling_the_model_is_told_to_keep_working(db, user_id): + """Exactly one of the two fragments ever appears. With no budget the model + must not be left with nothing said about when to stop -- and must certainly + not be told it has a budget of two hundred, which it would ration.""" + from lembas.db.models import User + from lembas.services import harness + from lembas.services import tools as tools_service + + chat_id, _message_id = _chat_with_tools(db, user_id) + chat = db.get(Chat, chat_id) + user = db.get(User, user_id) + offered = tools_service.resolve_tools(db, chat, user).schemas + + values = harness.context_variables(db, user, offered, chat) + assert values["round_budget"] == "" + assert values["unbounded"] == "yes" + + text = harness.compose(db, user, offered, chat) + assert "Keep working until the task is actually done" in text + assert "rounds of tool calls before you have to" not in text + + +async def test_with_a_ceiling_the_model_is_told_the_budget(db, user_id): + from lembas.db.models import User + from lembas.services import harness + from lembas.services import tools as tools_service + + settings_store.update(db, {"max_chat_rounds": 3}) + chat_id, _message_id = _chat_with_tools(db, user_id) + chat = db.get(Chat, chat_id) + user = db.get(User, user_id) + offered = tools_service.resolve_tools(db, chat, user).schemas + + values = harness.context_variables(db, user, offered, chat) + assert values["round_budget"] == "3" + assert values["unbounded"] == "" + + text = harness.compose(db, user, offered, chat) + assert "at most 3 rounds" in text + assert "Keep working until the task is actually done" not in text + + async def test_a_prompt_queued_during_a_one_round_reply_waits_for_its_own( db, user_id, monkeypatch ):