A ceiling for a chat, and a nudge for an agent that stops early
MAX_ROUNDS = 1 was wrong, and wrong in a way worth writing down. The loop already 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 termination condition every agentic harness uses. A round limit was never a schedule; it exists to catch the case where the model never says so. One is low enough to stop being a ceiling and start being a schedule: it overrode the model's judgement on every single turn. And it broke something concrete. 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. That is not an edge case, it is the library working at half depth, and I understated it as "cannot search the web and then read a result" when the change went in. It is a setting now, under General, default 5, with 0 meaning no ceiling. The loop and the harness both read settings_store.chat_rounds, so the model is never told a budget that is not its own; tools.MAX_ROUNDS is the fallback for callers with no session and a test pins the two equal. core.rounds goes back to naming the number, and vanishes entirely when there is no ceiling rather than promising zero rounds. The other half of "let it decide how long to go": an agent reply that ends while its plan still has open tasks is asked once to carry on. Only 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, and arguing with it would be guessing. At most twice in a row, with the count reset the moment it calls a tool again, so the bound is on consecutive stops rather than on stops in total. Never in Plan mode and never past plan_submit, which ends the turn on purpose. Giving up is recorded as an event rather than left silent. The model's own words go back with the nudge, which turned up a real bug on the way: ReasoningSplitter holds back a few characters against a <think> tag split across chunks, so round_text at the end of a round was missing its tail. That text is echoed as an assistant turn for tool rounds too, so a model has been occasionally asked to continue from a transcript where it trailed off mid-sentence. Flushed per round now. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
+16
-4
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user