glm53: replay a bare tool-call turn as the model wrote it (#1576) - #1583
Merged
Merged
Conversation
The official template writes "\n<tool_call>". GLM-5.3 does not: on an assistant turn that is nothing but a tool call it writes "</think><tool_call>", with no newline between them. render_chat_glm53 wrote the newline anyway, so the replayed prefix carried one token the model had never produced. One token is the whole cost. The reuse gate in glm53.c is all-or-nothing, so the next turn threw away the entire cached prefix and re-prefilled from scratch. The reporter measured that on a 3k-token agent history at 2.3 tok/s: twenty minutes, every turn after a bare tool call. A turn that also carries text is left exactly as it was, and that is not an oversight. There the model's own trailing newline is stripped by .strip() and put back by the renderer, the tokens line up, and it is the case that works today. Only the bare turn changes, which is the only one that was broken. tests/test_openai_server.py asserts both halves: "</think><tool_call>" with no newline for the bare turn, and the separator still present for the turn with text, so a later tidy-up cannot quietly break the working case while fixing nothing.
vrinek
added a commit
to vrinek/colibri
that referenced
this pull request
Sep 17, 2026
GLM-5.3 writes `</think><tool_call>` on a turn that is nothing but a tool call. The official template writes `</think>` NEWLINE `<tool_call>`, and render_chat_glm53 is pinned to the template, so the replayed prefix carried a token the model had never produced. The gate is all-or-nothing, so the next turn re-prefilled everything: on a 3k-token agent history at 2.3 tok/s, about twenty minutes, on every turn after a bare tool call (JustVugg#1576). arm_equiv registers that one pair, encoded with the serve tokenizer beside arm_stops, which builds its table the same way and for the same reason -- the ids depend on the model. A pair that will not encode is reported and skipped: without it the engine is slow, not wrong. slot_shared is gone because coli_equiv_reconcile with an empty table is exactly it. Measured on the reporter's box, with the renderer deliberately left as the template has it, so the engine carries the whole fix: REUSE 1 0 170 REUSE 2 182 192 turn 2 in 10 s, was 70 s JustVugg#1583 fixes the renderer, which is the right fix for this particular disagreement and lands the common case. This is the complement the report also asked for: the next divergence of the same shape costs a table entry rather than every prefix in every conversation. Full suite back to its baseline on this host: test-c green, 722 python tests with the two failures that predate the branch (a CUDA test that wants an NVIDIA GPU, and JustVugg#1317's tensor diagnostic). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QYFXXeLqLiezZJnjTFYPw4
Bangkah
approved these changes
Sep 17, 2026
Bangkah
left a comment
Bangkah
left a comment
There was a problem hiding this comment.
Surgical patch preventing KV cache invalidation on bare GLM-5.3 tool calls by stripping template newlines. Clean conditional logic, solid test coverage, zero regression risk. LGTM.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #1576.
The one token
The official template writes
"\n<tool_call>". GLM-5.3 does not: on an assistant turn that is nothing but a tool call it writes</think><tool_call>, with no newline between them.render_chat_glm53wrote the newline anyway, so the replayed prefix carried a token the model had never produced.That single token is the whole cost. The reuse gate in
glm53.cis all-or-nothing, so the following turn discarded the entire cached prefix and re-prefilled from scratch. On the reporter's box — a 3k-token agent history at 2.3 tok/s — that is twenty minutes, on every turn after a bare tool call.What changes and what does not
Only the bare turn. A turn that also carries text keeps its separator, and that is deliberate rather than an omission: there the model's own trailing newline is stripped by
.strip()and put back by the renderer, the tokens already line up, and it is the case that works today. Touching it would break the half that works while fixing nothing.Verification
tests/test_openai_server.pygains a test that asserts both halves:</think><tool_call>with no newline for the bare turn, and the separator still present for the turn with text. The second assertion is the one that matters in a year — it stops a later tidy-up from quietly removing the newline everywhere.python3 -m pytest tests/test_openai_server.py— 172 passed, 13 subtests.Not addressed here
The report also notes that the gate is all-or-nothing, so any future divergence of this kind costs the whole prefix rather than the tail. That is a separate change to
glm53.cwith a different risk profile, and this PR deliberately does not attempt it: the renderer was wrong, and the renderer is what is fixed.