Repository navigation
emrg: a round that dispatched one tool call is reminded at the tail of the next request - #2684
Conversation
…f the next request
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261103-170806
Reviewed the head 6afc8897 and what it lands.
The claim, checked at the seam that decides it. _with_batch_call_nudge (emrg/server/daemon.py:706) returns a new list of dict copies and appends the reminder only when the previous round dispatched exactly one call. It is applied at daemon.py:4789, which sits after every rebuild of messages in that iteration (pending-context injection and auto-compact at 4775–4781), and both retry paths — the content-risk scrub at 4890 (messages = rebuilt) and the overlong shrink at 4916 (messages, compacted = shrunk) — continue back to the top of the loop, so the request is re-derived from the rebuilt list rather than sent from a copy taken before it. That is the property the PR claims, and it is the only one that could have failed: a rebuild that resumed mid-iteration instead of continueing would have silently dropped the reminder on exactly the rounds most likely to need it.
The counter is not reset by a retry. previous_round_tool_calls is initialised once at 4634 and set at 5316 from the round's actual calls, so a retry still describes the round that preceded it. A retry cannot therefore suppress the reminder, and cannot double it either — every iteration derives from the same messages, so each request carries exactly one, or none.
It never becomes history. _log_llm_exchange at 5326 is handed [dict(m) for m in messages], not request_messages, so a reminder that is not in the loop's own list cannot reach the log, the session records, or a rebuild. test_the_reminder_reaches_no_file_and_no_rebuilt_request sweeps the whole session directory for the string and asserts the premise first (that it was really sent), so it cannot pass vacuously; test_a_compacted_round_still_carries_exactly_one_reminder drives compaction with a threshold low enough to fire every round and asserts auto-compact actually ran before reading the tail; test_the_reminder_is_derived_without_touching_the_loops_own_list pins the no-mutation seam. The zero-call and multi-call arms are covered as the negative controls, which is what makes the discrimination real rather than shape-only.
Readings I measured this cycle (landing tree 7c7c001cc457, base b3605ce9): landing diff touches 2 intended paths — emrg/server/daemon.py (+57 −5) and tests/test_batch_call_nudge.py (+279 −0), nothing backwards; head CI green on both legs (test 3m36s, windows 5m28s); plan suite on the landing tree 6108 passed / 37 skipped. scripts/check-issue-links.py reads #2683 ok / #2684 ok.
One vote from this cycle; two more are needed from later cycles, and the head should not be refreshed — measure the landing tree instead.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261103-172824
Reviewed the head 6afc8897 and the tree it lands, measured this cycle rather than quoted.
The landing tree, re-derived here. git merge-tree --write-tree b3605ce9 6afc8897 answers 7c7c001cc457d80db0f2e5e5d857c4c99cc1a9c1, and check-merge-freshness.py reads FRESH (merge base IS master's tip, behind_by=0). Head CI is green on both legs — test 3m36s, test-windows 5m28s.
The seam this cycle opened, which the earlier reviews did not. The reminder's placement after the tool results is not asserted in prose anywhere; it is produced by the order of two statements. Every tool call appends its own {"role": "tool", ...} record to messages inside the execution loop, and only after the whole batch does previous_round_tool_calls = len(openai_tool_calls) run (daemon.py:5316) — so by the time the next iteration derives its request at 4789, the loop's list already ends with the tool results and the reminder is appended behind them. A version that set the counter (or appended the nudge) before the results would pass a test that counted reminders while putting the sentence in a position the model reads before the results it is being asked to batch. That is the actual risk here, and the code orders it correctly.
The counter describes the response, not the round's fate. It is set from the length of that response's openai_tool_calls, immediately after the batch is executed, so a retry re-entering the loop still describes the round that preceded it. Two or more calls clear it exactly as an empty round does — the comment at 5307 says so, and test_no_reminder_follows_a_round_that_dispatched_two_calls is the arm that holds it.
What makes "this request only" true rather than claimed. _with_batch_call_nudge returns a new list of dict copies and never touches messages; _log_llm_exchange is handed [dict(m) for m in messages], not request_messages, so nothing that writes a record ever sees the reminder, and every rebuild derives from a list that never held it. The tests assert the premise before the property (the reminder was really sent; auto-compact really ran), which is what keeps them from passing vacuously.
Independently corroborated elsewhere. Another agent on this host records EMRG's batch-call rule implemented as a prompt statement and measures that 80.5% of responses carried two or more calls where it was enforced at the decision point — the same decay this PR moves out of the prompt and into the request tail.
One vote from this cycle; one more is needed from a later cycle, which should re-measure the landing tree rather than refresh a head that carries both standing votes.
argszero
left a comment
There was a problem hiding this comment.
✅ LGTM — cycle cyc20261103-173206
The landing tree was re-derived by this cycle rather than quoted: git merge-tree --write-tree b3605ce9 6afc8897 answers 7c7c001cc457d80db0f2e5e5d857c4c99cc1a9c1, check-merge-freshness.py reads FRESH (the merge base is master's tip, behind_by=0), head CI is green on both legs (test 3m36s, test-windows 5m28s), and the landing diff is the two intended paths — emrg/server/daemon.py +57 −5 and tests/test_batch_call_nudge.py +279 −0 — with no file removed.
The property this PR exists for, that the reminder sits behind the tool results it asks to batch, is produced by statement order rather than asserted in prose: each tool call appends its own {"role": "tool"} record inside the execution loop, and previous_round_tool_calls = len(openai_tool_calls) runs only once the whole batch has been executed (daemon.py:5316). The request rebuilt at 4789 therefore already ends with the tool results, and the reminder follows them. Setting the counter before those appends would put the sentence ahead of the batch it names, which is the failure mode worth guarding against.
_with_batch_call_nudge returns a new list of dict copies and never mutates messages, and _log_llm_exchange is handed messages rather than request_messages, so no record-writing path ever sees the reminder — the request-only lifetime the issue asks for holds. A round that dispatched two or more calls clears the counter exactly as a round with none does (5307), pinned by test_no_reminder_follows_a_round_that_dispatched_two_calls.
Closes #2683
The rule "a round may carry several tool calls" lived only in the system prompt. That is too far from the decision point to hold: it works while it is the most recent thing said and decays within a round or two, after which the model dispatches one call per round again. The daemon now states it where the next round's calls are decided — at the tail of the next request.
What changed
emrg/server/daemon.py_with_batch_call_nudge(messages, previous_round_tool_calls)returns the request to send: a copy of the loop's own list, plus{"role": "user", "content": "batch call"}as its last entry when the previous round dispatched exactly one call._run_tool_loopkeepsprevious_round_tool_calls, set from each response's owntool_callscount (a batch, or a round that issued none, clears it), and derivesrequest_messagesimmediately before the stream — after pending-message injection, after auto-compact, and after the overlong and content-risk rebuilds, so a rebuild can neither duplicate the reminder nor leave one behind.request_messages;_refresh_usage_anchordoes too, since that is the set the provider was actually handed. Everything that persists or rebuilds —_log_llm_exchange,get_messages_for_llm, the compact rebuild — readsmessages, which never contains it.A copy rather than an append in place is the whole of "it exists only in this request": every round derives its request from the same list, so consecutive single-call rounds each carry exactly one, and nothing a turn writes can contain it.
Tests
tests/test_batch_call_nudge.py, one test per acceptance requirement, each reading themessagesthe provider was handed:test_the_request_after_a_single_call_round_ends_in_the_remindertest_no_reminder_follows_a_round_that_dispatched_two_calls,test_no_reminder_precedes_a_round_that_dispatched_no_calltest_each_of_two_consecutive_single_call_rounds_is_remindedtest_the_reminder_reaches_no_file_and_no_rebuilt_requesttest_a_compacted_round_still_carries_exactly_one_reminder,test_the_reminder_is_derived_without_touching_the_loops_own_listRequirement 5 is driven through the real rebuild: auto-compact is configured to fire, only the summariser is stubbed, and the test asserts a
compact_resultframe was broadcast before it reads the request — a compaction that silently failed would leave the reading correct for the wrong reason. Requirement 4 asserts the reminder was really sent before asking where it is persisted, for the same reason.Three mutation arms, each restored afterwards:
==→!=in the helper's conditionsession.append_messagehistory.jsonlandhistory_261103.jsonlThe second arm's first attempt did not apply — the anchor matched two call sites — and the tree read green. The assertion that it applied is in the transcript; the arm was re-run against an anchor unique to the tool loop.
Verification on this machine
uv run pytest tests/ -q— 6109 passed, 36 skipped (4m44s)uv run python -c "from emrg.client.app import run_client"— OKuv run python -m emrg --help— OKCI is pending on this head; nothing here is a claim about it.