One round for a chat, as many as it takes for an agent
Two different jobs were sharing one number. 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. MAX_ROUNDS is 1 now. 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. An agent chat is sized by Limits instead, where steps is now a runaway backstop and not a working budget. It was 40 and it was reached -- a step count low enough to be the thing that ends a reply is a count that ends it halfway. What bounds one now is the wall clock and a new completion-token ceiling, with zero meaning no ceiling, the same convention index_chars already uses. That ceiling would have been decorative. 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. So _written takes the larger of reported and estimated, and there is a test that runs the whole thing against a stream reporting no usage at all. A limit that works on OpenAI and silently does nothing everywhere else is the worst kind: one that looks configured. core.rounds could not stay one fragment. "You get at most N rounds" is not the same sentence with a different number in it -- a model told it has a budget rations it and stops early to report progress, which is exactly the behaviour that strands a long piece of work. So it splits: core.rounds keeps the one-round case and gates on a new round_budget variable that _agent_values blanks, and core.keep_working says the other thing to an agent chat. A queued message during a one-round reply is now never taken mid-reply -- there is no work under way to steer -- and falls through to _drain, which gives it a reply of its own. No code change went with that; it falls out of the guard, and there is a test so that "it happens to work" and "it is meant to work" stop looking the same. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -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):
|
||||
|
||||
@@ -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.
|
||||
|
||||
@@ -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):
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user