diff --git a/CLAUDE.md b/CLAUDE.md index 66293a4..a23cf50 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -20,7 +20,7 @@ lembas info # paths + counts, useful when confused lembas secret-key # generate LEMBAS_SECRET_KEY lembas create-admin # create or promote an admin -pytest # 1195 tests, ~70s +pytest # 1206 tests, ~70s # PLAN.md tracks what is and is not built ruff check . # lint (line length 100) python scripts/build_artwork.py # regenerate artwork (SVG + PWA icons; @@ -767,14 +767,22 @@ search is enabled, the user has `tools.web_search`, **and** the model is flagged `tools` — sending a `tools` array to an endpoint without support fails the whole request, exactly as images do without `vision`. -**An ordinary chat gets ONE round; an agent chat runs until the work is done.** -`MAX_ROUNDS` is 1. A plain conversation asking a question is one round of looking -things up and then an answer, and 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. The trade is real and worth naming: a plain -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. +**A round ceiling is a ceiling, not a schedule.** The loop ends the moment a +round comes back with no tool calls — that is the model saying it has what it +needs, and it is the same termination condition every agentic harness uses. The +number only catches the case where it never says so: a small model that has +decided searching is the answer, searching until the context runs out at a full +request each. + +It was briefly 1, and that is the lesson. One 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 a ceiling of one left the library searchable and not readable. +`settings_store.chat_rounds()` is the number now, default 5, **0 meaning no +ceiling** (the loop falls back to `MAX_TOOL_ROUNDS`, a runaway backstop). +`tools.MAX_ROUNDS` is only the fallback for callers with no session, and a test +pins the two equal. An agent chat is sized by `agent/policy.py:Limits` instead, where **`steps` is a runaway backstop and not a working budget**. It was 40 and it was reached; a step @@ -782,7 +790,9 @@ count low enough to be the thing that ends a reply is a count that ends it halfway. What actually bounds one is the wall clock and `completion_tokens`. These are different sentences rather than the same sentence with a different number in it, which is why `core.rounds` and `core.keep_working` are two -fragments gated on `round_budget` rather than one with `{{max_rounds}}` in it. +fragments gated on `round_budget` — blank in an agent chat, and blank again when +an administrator has set no ceiling, so the fragment vanishes rather than +promising zero rounds. **The token ceiling would have worked on OpenAI and silently done nothing elsewhere.** `generation.completion_tokens` is only populated when the endpoint @@ -791,6 +801,25 @@ estimate is computed once, in `_run`'s `finally:`, long after the loop that need it. `_written()` takes `max(reported, estimated)` so the limit fires everywhere. The worst kind of limit is one that looks configured. +**A model that stops is believed, unless its own plan says otherwise.** +`core.keep_working` is the cheap half of stopping-halfway; `generation._nudge` +is the other half, and it only fires where there is something objective to check +against — an open task on the chat's plan. No plan means nothing to be wrong +about, so a model with none that says it has finished is taken at its word. It +is asked at most `MAX_NUDGES` times **in a row** (the count resets the moment a +tool is called again), never in Plan mode, and never past `plan_submit` — that +ends the turn deliberately and nudging it would argue with the point of the +mode. Giving up is recorded as an event rather than left silent. The model's own +words go back with the nudge, or it is asked to carry on from a transcript in +which it never spoke. + +**A round's text is flushed before it is echoed back.** `ReasoningSplitter` +holds back a few characters against a `` tag split across chunks, so +`round_text` at the end of a round was missing its last words — and that text is +echoed as an assistant turn, both for a tool round and for a nudge. A turn +missing its tail is one the model is asked to continue from having apparently +trailed off mid-sentence. + **A chat can narrow what it may use, and can never widen it.** `Chat.scope_json` is filtered inside `resolve_tools` *after* the capability, permission and instance gates — exactly as `chat.knowledge_bases` narrows `knowledge_search` — diff --git a/src/lembas/api/admin.py b/src/lembas/api/admin.py index 12055b1..9a6a3ee 100644 --- a/src/lembas/api/admin.py +++ b/src/lembas/api/admin.py @@ -59,6 +59,7 @@ async def save_general( allow_signup: bool = Form(False), system_prompt: str = Form(""), compact_threshold: int = Form(95), + max_chat_rounds: int = Form(5), ) -> Response: """Save instance settings. @@ -77,6 +78,9 @@ async def save_general( "compact_threshold": ( 0 if compact_threshold <= 0 else min(max(compact_threshold, 50), 99) ), + # Floor of 0, not 1: zero is how "no ceiling" is said, and the loop + # falls back to a runaway backstop rather than to this number. + "max_chat_rounds": min(max(max_chat_rounds, 0), 100), }, ) log.info("registration %s by %s", "opened" if allow_signup else "closed", user.email) diff --git a/src/lembas/api/admin_agents.py b/src/lembas/api/admin_agents.py index a65e86a..a434912 100644 --- a/src/lembas/api/admin_agents.py +++ b/src/lembas/api/admin_agents.py @@ -78,6 +78,7 @@ async def save_agents( index_chars: int = Form(2000), instructions_enabled: bool = Form(False), instructions_chars: int = Form(4000), + nudge_unfinished: bool = Form(False), ) -> Response: settings_store.update( db, @@ -110,6 +111,7 @@ async def save_agents( "index_chars": min(max(index_chars, 0), 20_000), "instructions_enabled": instructions_enabled, "instructions_chars": min(max(instructions_chars, 0), 20_000), + "nudge_unfinished": nudge_unfinished, }, key=settings_store.AGENTS, ) diff --git a/src/lembas/services/generation.py b/src/lembas/services/generation.py index 9a4b97d..cafa85b 100644 --- a/src/lembas/services/generation.py +++ b/src/lembas/services/generation.py @@ -30,7 +30,7 @@ from lembas.db.models import KIND_AGENT, ROLE_ASSISTANT, ROLE_USER, Chat, Messag from lembas.db.session import session_scope from lembas.services import chat as chat_service from lembas.services import compaction as compaction_service -from lembas.services import interaction, tokens, tool_labels +from lembas.services import interaction, settings_store, tokens, tool_labels from lembas.services import metrics as metrics_service from lembas.services import prompts as prompts_service from lembas.services import tools as tools_service @@ -58,6 +58,17 @@ RENDER_INTERVAL = 0.1 # gets the final frames, then are pruned. KEEP_FINISHED = timedelta(minutes=5) +# What "no ceiling" resolves to. A setting of 0 means an administrator does not +# want a round limit, but a loop needs *some* stop or a model stuck calling one +# cheap tool runs until the process does. This is high enough never to be +# reached by anything but that. +MAX_TOOL_ROUNDS = 200 + +# How many times in a row a reply that stopped with plan tasks outstanding may +# be told to carry on. Two, so a model that genuinely has nothing left to do can +# say so and be believed rather than argued with indefinitely. +MAX_NUDGES = 2 + @dataclass class Generation: @@ -142,6 +153,10 @@ class Generation: # -- the one frame that reaches a browser after a reply is over. drained: bool = False injected_ids: list[str] = field(default_factory=list) + # How many times *in a row* this reply has ended with plan tasks still open + # and been told to carry on. Reset the moment it calls a tool again, so the + # count is of consecutive stops rather than of stops in total. + nudges: int = 0 def touch(self) -> None: self.version += 1 @@ -363,11 +378,18 @@ async def _run(generation: Generation) -> None: # vision model, a plain string to anything else, or the endpoint # rejects the whole request. vision = chat_service.model_supports(db, chat, "vision") + 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 - budget = limits.steps if limits else tools_service.MAX_ROUNDS + # 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 + # means an ordinary chat has no ceiling either; `steps` is already a + # runaway backstop rather than a budget, so an agent chat is bounded by + # tokens and the clock instead. + budget = limits.steps if limits else (chat_rounds or MAX_TOOL_ROUNDS) for round_number in range(budget + 1): generation.rounds = round_number + 1 @@ -441,9 +463,47 @@ async def _run(generation: Generation) -> None: # Let followers and other tasks run between chunks. await asyncio.sleep(0) + # The round is over, so anything the splitter is still holding back + # against a `` tag split across chunks is not a tag. Flushed + # here rather than only after the loop, because `round_text` is + # echoed back as an assistant turn -- for a tool round and for a + # nudge alike -- and a turn missing its last few words is a turn the + # model is asked to continue from having apparently trailed off. + for kind, piece in splitter.flush(): + if kind == REASONING: + generation.reasoning.append(piece) + else: + generation.content.append(piece) + round_text.append(piece) + calls = accumulator.calls if generation.stopped or not calls: - break + # The model says it is done. Believe it -- unless this is an + # agent chat whose plan still has work in it, in which case ask + # once. `_nudge` returns the turn to send, or None. + added = _nudge( + generation, + tool_context, + enabled=nudge_enabled, + stopped=generation.stopped, + round_number=round_number, + budget=budget, + ) + if added is None: + break + # Its own words go back with the nudge. Without the assistant + # turn the model is asked to carry on from a transcript in which + # it never spoke, and repeats itself. + said = "".join(round_text).strip() + messages = [*payload["messages"]] + if said: + messages.append({"role": "assistant", "content": said}) + payload = {**payload, "messages": [*messages, added]} + continue + + # Something was called, so whatever it said it had finished, it had + # 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 @@ -631,7 +691,6 @@ async def _warm_project(generation: Generation) -> None: the new one was silently never warmed on any chat that had a listing -- which is to say, on every chat after the first reply. """ - from lembas.services import settings_store from lembas.services.agent import index as index_service from lembas.services.agent import instructions as instructions_service from lembas.services.agent import session as agent_session @@ -758,6 +817,89 @@ def _gave_up(generation, why: str) -> None: generation.touch() +def _nudge( + generation: Generation, + context, + *, + enabled: bool, + stopped: bool, + round_number: int, + budget: int, +) -> dict | None: + """The turn telling an agent to carry on, or None to let the reply end. + + A model that stops with work outstanding is the failure `core.keep_working` + is worded against, and prompting is the cheaper half of the fix. This is the + other half, and it only fires where there is something objective to check + against: an open task on the chat's own plan. Without a plan there is + nothing to be wrong about, so nothing happens -- a model that has genuinely + finished must be able to say so and be believed. + + Every "no" is a plain None: + + * the setting is off, or the reply was stopped, or it errored; + * this is not an agent chat, or is one in Plan mode -- `plan_submit` ends + the turn deliberately and nudging past it would be arguing with the whole + point of the mode; + * there is no plan, or every task on it is done or dropped; + * there is no round left to carry on in, or it has already been asked + MAX_NUDGES times in a row. + + The last one is recorded rather than silent. A reply that stopped twice with + work outstanding is worth being able to see afterwards. + """ + agent = getattr(context, "agent", None) + if not enabled or stopped or generation.error or agent is None: + return None + if agent.mode == agent_policy.MODE_PLAN or generation.plan_final: + return None + + plan = generation.plan if generation.plan is not None else agent.plan + open_tasks = [ + task + for phase in (plan or {}).get("phases", []) + for task in phase.get("tasks", []) + if task.get("status") not in ("done", "dropped") + ] + if not open_tasks: + return None + if round_number >= budget: + return None + + if generation.nudges >= MAX_NUDGES: + generation.tool_events.append( + { + "name": "plan_update", + "kind": "plan", + "status": "error", + "error": ( + f"Stopped with {len(open_tasks)} task(s) still open on the " + f"plan, after being asked twice to carry on." + ), + "results": [], + } + ) + generation.touch() + return None + + generation.nudges += 1 + remaining = "\n".join(f"- {task['id']} {task['text']}" for task in open_tasks[:8]) + # A user turn, and phrased as the reader would phrase it. Everything else + # this codebase injects is quoted and attributed because it came out of a + # file or a machine; this is the application speaking on the reader's behalf + # about the reader's own plan, which is the one case where that is honest. + return { + "role": "user", + "content": ( + "The plan still has work in it:\n" + f"{remaining}\n\n" + "Carry on with the next one. If something here cannot be done, or is " + "no longer worth doing, mark it dropped with plan_update and say why " + "— do not leave it open and stop." + ), + } + + def _written(generation: Generation) -> int: """How much this reply has written so far, in tokens, reported or estimated. diff --git a/src/lembas/services/harness.py b/src/lembas/services/harness.py index fe0254b..02f7eb8 100644 --- a/src/lembas/services/harness.py +++ b/src/lembas/services/harness.py @@ -124,12 +124,16 @@ def context_variables( "instance_name": str(settings_store.get(db, "instance_name") or "LLeMbas"), "user_name": (user.name or "") if user is not None else "", "model_name": "", - "max_rounds": str(tools_service.MAX_ROUNDS), + # What this request will actually allow, so the model is not told a + # number that is not its own. `tools_service.MAX_ROUNDS` is only the + # fallback for callers with no session. + "max_rounds": str(settings_store.chat_rounds(db) or 0), # 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), + # chat has a ceiling worth planning within, an agent chat is told to + # keep going instead, and those are different sentences rather than the + # 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 ""), "memory_limit": str(memories_service.MAX_MEMORY_CHARS), "tool_names": _tool_names(offered), "memories": memories_service.block(db, user) if "memory" in families else "", diff --git a/src/lembas/services/prompts.py b/src/lembas/services/prompts.py index c844221..90171fb 100644 --- a/src/lembas/services/prompts.py +++ b/src/lembas/services/prompts.py @@ -628,17 +628,21 @@ BUILTIN: tuple[Fragment, ...] = ( order=110, when_tools=True, 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.", + variables=("max_rounds",), + hint="An ordinary chat only, and only when it has a ceiling at all. " + "What is worth telling a model with a budget is different in kind " + "from what is worth telling one that should keep going until the work " + "is done — not the same sentence with a different number in it — so " + "this is gated on `round_budget`, which `_agent_values` blanks and " + "which is also blank when an administrator has set no ceiling. The " + "agent case is its own fragment below.", default=( - "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." + "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, so " + "ask for everything you need at once rather than one thing at a time. " + "Plan within that: two careful searches beat six that run out halfway. If " + "what comes back is not enough, say what you would look up next rather " + "than answering as though it were." ), ), Fragment( diff --git a/src/lembas/services/settings_store.py b/src/lembas/services/settings_store.py index 746a178..faf0733 100644 --- a/src/lembas/services/settings_store.py +++ b/src/lembas/services/settings_store.py @@ -21,6 +21,11 @@ from lembas.db.models import Setting GENERAL = "general" 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 SEARCH = "search" PROMPTS = "prompts" AGENTS = "agents" @@ -42,6 +47,17 @@ def _general_defaults() -> dict[str, Any]: # Never fires for a model whose context_length is 0, since that is # "unknown" rather than "small". See services/compaction.py. "compact_threshold": 95, + # How many rounds of tool calls an ordinary chat may take before it has + # to answer with words. A **ceiling**, not a schedule: the loop already + # ends the moment a round comes back with no tool calls, which is the + # 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, } @@ -116,6 +132,11 @@ def _agents_defaults() -> dict[str, Any]: # untrusted, and the fragment carrying it is where that is dealt with. "instructions_enabled": True, "instructions_chars": 4000, + # Whether a reply that ends while its plan still has open tasks is told + # once to carry on. Only ever fires against a plan, because that is the + # one thing there is to be objectively wrong about -- a model with no + # plan that says it has finished is believed. + "nudge_unfinished": True, } @@ -214,6 +235,20 @@ def get(db: DBSession, name: str, *, key: str = GENERAL) -> Any: return get_group(db, key).get(name) +def chat_rounds(db: DBSession) -> int: + """The ceiling on an ordinary chat's rounds of tool calls, clamped. + + Zero is meaningful and is not clamped away: it means "no ceiling", the same + convention `index_chars` and `max_completion_tokens` use. Read through here + rather than from the group directly so the loop and the harness cannot + disagree about the number the model is told. + """ + stored = get_group(db, GENERAL).get("max_chat_rounds") + if stored is None: + return DEFAULT_CHAT_ROUNDS + return min(max(int(stored), 0), 100) + + def update(db: DBSession, changes: dict[str, Any], *, key: str = GENERAL) -> dict[str, Any]: """Merge changes into a settings group and persist them.""" row = db.get(Setting, key) diff --git a/src/lembas/services/tools.py b/src/lembas/services/tools.py index 5460dce..9b77c87 100644 --- a/src/lembas/services/tools.py +++ b/src/lembas/services/tools.py @@ -50,16 +50,22 @@ log = logging.getLogger(__name__) # 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`. +# A **ceiling, not a schedule.** The loop ends the moment a round comes back +# with no tool calls -- that is the model saying it has what it needs, and it is +# the same termination condition every agentic harness uses. This number only +# catches the case where it never says so: a small model that has decided +# searching is the answer, searching until the context runs out at a full +# request each. # -# 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 +# 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. +# +# `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 # 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/src/lembas/web/templates/admin/agents.html b/src/lembas/web/templates/admin/agents.html index 585170d..d2f967b 100644 --- a/src/lembas/web/templates/admin/agents.html +++ b/src/lembas/web/templates/admin/agents.html @@ -155,6 +155,21 @@ +
+ +

+ Only ever against a plan, and only while tasks on it are still open — + that is the one thing there is to be objectively wrong about. A reply + with no plan that says it has finished is believed. It is asked at most + twice in a row, and if it stops a third time that is recorded in the + transcript rather than argued with. +

+
+
+
+

Tool calls in an ordinary chat

+

+ A model ends its own turn the moment it stops asking for tools — that is + it saying it has what it needs, and nothing here overrides it. This is a + ceiling for the case where it never says so. +

+
+ + +

+ Several tools can be called in one round, so this is not a count of + tools. Leave room for at least two: knowledge_get and + notes_get read a document by an id a search + returned, so a ceiling of one leaves the library searchable and not + readable. 0 means no ceiling, which is how an agent chat + already works — those are bounded under + Agents by time and tokens instead. +

+
+
+

Registration diff --git a/tests/test_agent_plan.py b/tests/test_agent_plan.py index 42cce3c..63443a7 100644 --- a/tests/test_agent_plan.py +++ b/tests/test_agent_plan.py @@ -351,3 +351,160 @@ def test_a_plan_pointer_at_another_chats_message_is_ignored(db, owner): db.commit() assert session.resolve(db, chat, owner).plan == {} + + +# --- Being asked to carry on ------------------------------------------------------ +def _stub(rounds, seen): + async def stream_chat(_endpoint, payload): + seen.append(payload) + for chunk in rounds[min(len(seen) - 1, len(rounds) - 1)]: + yield chunk + + return stream_chat + + +def _text(text): + return {"choices": [{"delta": {"content": text}}]} + + +def _call(name, arguments): + return {"choices": [{"delta": {"tool_calls": [ + {"index": 0, "id": "c1", "function": {"name": name, "arguments": arguments}}]}}]} + + +def _reply(db, chat): + from lembas.services import generation as generation_service + + db.add(Message(chat_id=chat.id, role="user", content="do it", complete=True)) + db.commit() + assistant = Message(chat_id=chat.id, role=ROLE_ASSISTANT, content="", complete=False) + db.add(assistant) + db.commit() + return generation_service.Generation(chat_id=chat.id, message_id=assistant.id) + + +async def _run(monkeypatch, generation, rounds, seen): + from lembas.services import generation as generation_service + + monkeypatch.setattr(generation_service, "stream_chat", _stub(rounds, seen)) + + async def _no_title(*_a, **_k): + return "" + + monkeypatch.setattr("lembas.services.chat.generate_title", _no_title) + await generation_service._run(generation) + + +async def test_stopping_with_open_tasks_is_answered_with_carry_on(db, owner, monkeypatch): + """The half prompting cannot do. core.keep_working tells it not to stop + halfway; this is what happens when it does anyway.""" + chat = _agent_chat(db, owner, mode=policy.MODE_AUTO) + _with_plan(db, chat, V1) + generation = _reply(db, chat) + + seen: list[dict] = [] + await _run(monkeypatch, generation, [[_text("I have done the first bit.")], + [_text("All finished.")]], seen) + + # Three requests: the reply, then a nudge, then a second nudge -- this + # model never marks anything done, so the plan stays open and it is asked + # until MAX_NUDGES runs out. That it gives up is the next test. + assert len(seen) == 3, "it was asked again" + nudge = seen[1]["messages"][-1] + assert nudge["role"] == "user" + assert "still has work in it" in nudge["content"] + assert "Rotate it" in nudge["content"], "and says which tasks" + # Its own words go back with it, or it is asked to carry on from a + # transcript in which it never spoke. + assert seen[1]["messages"][-2]["content"] == "I have done the first bit." + + +async def test_a_finished_plan_is_believed(db, owner, monkeypatch): + chat = _agent_chat(db, owner, mode=policy.MODE_AUTO) + done = plans.build(title="x", phases=[{"title": "P", "tasks": ["a"]}]) + done["phases"][0]["tasks"][0]["status"] = "done" + _with_plan(db, chat, done) + generation = _reply(db, chat) + + seen: list[dict] = [] + await _run(monkeypatch, generation, [[_text("All done.")]], seen) + + assert len(seen) == 1 + + +async def test_a_chat_with_no_plan_is_never_nudged(db, owner, monkeypatch): + """There is nothing to be objectively wrong about, so a model that says it + has finished is believed.""" + chat = _agent_chat(db, owner, mode=policy.MODE_AUTO) + generation = _reply(db, chat) + + seen: list[dict] = [] + await _run(monkeypatch, generation, [[_text("All done.")]], seen) + + assert len(seen) == 1 + + +async def test_plan_mode_is_never_nudged(db, owner, monkeypatch): + """plan_submit ends the turn deliberately. Nudging past it would argue with + the whole point of the mode.""" + chat = _agent_chat(db, owner, mode=policy.MODE_PLAN) + _with_plan(db, chat, V1) + generation = _reply(db, chat) + + seen: list[dict] = [] + await _run(monkeypatch, generation, [[_text("Here is what I would do.")]], seen) + + assert len(seen) == 1 + + +async def test_it_gives_up_after_two_and_says_so(db, owner, monkeypatch): + """A model that has nothing left to do must be able to say so and be + believed rather than argued with indefinitely -- and a reply that stopped + twice with work outstanding is worth being able to see afterwards.""" + chat = _agent_chat(db, owner, mode=policy.MODE_AUTO) + _with_plan(db, chat, V1) + generation = _reply(db, chat) + + seen: list[dict] = [] + await _run(monkeypatch, generation, [[_text("Nothing more from me.")]], seen) + + from lembas.services import generation as generation_service + + assert len(seen) == generation_service.MAX_NUDGES + 1 + assert generation.tool_events[-1]["status"] == "error" + assert "asked twice" in generation.tool_events[-1]["error"] + + +async def test_calling_a_tool_again_resets_the_count(db, owner, monkeypatch): + """The count is of consecutive stops. A model that stops, is nudged, does + some work and stops again has not run out of patience.""" + chat = _agent_chat(db, owner, mode=policy.MODE_AUTO) + _with_plan(db, chat, V1) + generation = _reply(db, chat) + + seen: list[dict] = [] + await _run( + monkeypatch, + generation, + [[_text("Pausing.")], [_call("file_list", '{"path": "."}')], [_text("Pausing again.")]], + seen, + ) + + # Without the reset this run would end after two nudges, at three requests. + # The tool call in the middle clears the count, so it gets more than that -- + # which is the property, and does not depend on where the stub stops. + assert len(seen) > 3 + + +async def test_the_switch_turns_it_off(db, owner, monkeypatch): + from lembas.services import settings_store + + settings_store.update(db, {"nudge_unfinished": False}, key=settings_store.AGENTS) + chat = _agent_chat(db, owner, mode=policy.MODE_AUTO) + _with_plan(db, chat, V1) + generation = _reply(db, chat) + + seen: list[dict] = [] + await _run(monkeypatch, generation, [[_text("All done.")]], seen) + + assert len(seen) == 1 diff --git a/tests/test_generation_tools.py b/tests/test_generation_tools.py index 7c14153..696126e 100644 --- a/tests/test_generation_tools.py +++ b/tests/test_generation_tools.py @@ -151,6 +151,10 @@ async def test_a_model_that_only_ever_calls_tools_is_stopped(db, user_id, monkey chat_id, message_id = _chat_with_tools(db, user_id) monkeypatch.setattr("lembas.services.search.run", _empty_search) + # Set explicitly rather than read from the constant: a test that reads the + # number under test passes whatever the number becomes, which is the + # assertion nobody wanted. + settings_store.update(db, {"max_chat_rounds": 3}) payloads = [] monkeypatch.setattr( @@ -163,30 +167,27 @@ 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) - # 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 + # The ceiling, plus the round that has to answer with words. + assert len(payloads) == 4 # 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"] + assert "3 rounds" 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. + """`_inject` only takes a prompt in while there is a round left to answer + in. With a ceiling of one 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 + One is no longer the default, but it is still a setting somebody can choose, + and "it happens to work" and "it is meant to work" look the same until somebody changes the guard. """ + settings_store.update(db, {"max_chat_rounds": 1}) from lembas.db.models import Message from lembas.services import chat as chat_service @@ -381,3 +382,44 @@ async def test_a_custom_tool_runs_inside_the_loop(db, user_id, monkeypatch, mock assert tool_turns[0]["content"] == "Sunny in Minas Tirith." assert generation.tool_events[0]["kind"] == "custom" assert generation.tool_events[0]["label"] == "Weather" + + +def test_the_default_and_the_fallback_cannot_drift(db): + """`tools.MAX_ROUNDS` exists for callers with no session; the setting is + what the loop and the harness read. Two numbers meaning one thing is how + a model gets told a budget it does not have.""" + assert tools_service.MAX_ROUNDS == settings_store.DEFAULT_CHAT_ROUNDS + assert settings_store.chat_rounds(db) == tools_service.MAX_ROUNDS + + +async def test_a_ceiling_of_zero_does_not_mean_zero_rounds(db, user_id, monkeypatch): + """It means no ceiling. Read carelessly it would mean the model never gets + to call anything, which is the opposite.""" + settings_store.update(db, {"enabled": True}, key=settings_store.SEARCH) + settings_store.update(db, {"max_chat_rounds": 0}) + chat_id, message_id = _chat_with_tools(db, user_id) + + monkeypatch.setattr("lembas.services.search.run", _empty_search) + payloads = [] + monkeypatch.setattr( + generation_service, + "stream_chat", + _stub_stream( + [[_tool_call_chunk("web_search", '{"query": "x"}')], [_text_chunk("Done.")]], + payloads, + ), + ) + 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 == "Done." + assert len(payloads) == 2, "it called a tool and then answered, uninterrupted" + + +def test_the_ceiling_is_clamped(db): + settings_store.update(db, {"max_chat_rounds": 9999}) + assert settings_store.chat_rounds(db) == 100 + settings_store.update(db, {"max_chat_rounds": -5}) + assert settings_store.chat_rounds(db) == 0 diff --git a/tests/test_harness.py b/tests/test_harness.py index 1acab53..57ce0ea 100644 --- a/tests/test_harness.py +++ b/tests/test_harness.py @@ -360,15 +360,27 @@ def test_a_plain_chat_is_told_nothing_about_files(db, owner): # --- 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.""" +def test_a_plain_chat_is_told_its_real_ceiling(db, owner): + """The number it is actually given, not a constant -- a model told it has + five rounds and cut off after three has been lied to about its own budget.""" + settings_store.update(db, {"max_chat_rounds": 3}) text = harness.compose(db, owner, _tools("web_search")) - assert "one round of tool calls" in text + assert "at most 3 rounds" in text assert "Keep working until the task is actually done" not in text +def test_no_ceiling_means_no_round_budget_is_claimed(db, owner): + """Zero is "no ceiling", and a fragment promising zero rounds would be worse + than none at all.""" + settings_store.update(db, {"max_chat_rounds": 0}) + text = harness.compose(db, owner, _tools("web_search")) + + # `core.interjection` also mentions rounds, so this asserts on the budget + # sentence rather than on the word. + assert "at most" 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