Skip to content

server: remember a tools-aware visible KV checkpoint after tool-call turns - #709

Open
Flor1an-B wants to merge 1 commit into
antirez:mainfrom
Flor1an-B:fix-tools-thinking-live-checkpoint
Open

server: remember a tools-aware visible KV checkpoint after tool-call turns#709
Flor1an-B wants to merge 1 commit into
antirez:mainfrom
Flor1an-B:fix-tools-thinking-live-checkpoint

Conversation

@Flor1an-B

Copy link
Copy Markdown

Fixes the root cause reported in #691 (and the same-shaped tool-continuation gap discussed in #609/#611, for the OpenAI-compatible chat/completions path once a conversation actually has tools).

Root cause

chat_history_uses_tool_context() makes render_deepseek_chat_prompt_text() always keep the opening <think> tag for a historical assistant turn once any tool schema is present — <think>{reasoning}</think> — rendering {reasoning} from whatever reasoning_content the client echoes back for that turn. Most OpenAI-compatible chat/completions clients never replay reasoning_content, so that renders as a literal <think></think>, while the live KV session actually contains the real sampled reasoning bytes for that turn.

The existing visible-checkpoint mechanism already handles exactly this mismatch for the toolless case (remember_thinking_checkpoint() / should_remember_thinking_checkpoint()), but both are explicitly gated off whenever request.has_tools is true, and the tool-call-finish path (canonicalize_tool_checkpoint(), or the raw-DSML-replay fallback when should_canonicalize_tool_checkpoint() is false) only ever calls thinking_live_clear() afterwards. So a tools-enabled conversation never gets a live-continuation match once a non-replaying client's rendered history diverges from the live tokens — which happens on essentially every turn, since thinking_live_visible_prefix_prompt() (the matching side) is already engine-agnostic and untouched by has_tools; only the remembering side was gated off.

Fix

Mirrors the toolless mechanism instead of introducing a new one:

  • should_remember_thinking_checkpoint() only excludes has_tools for GLM, whose history renderer keeps the opening <think> tag either way (append_glm_assistant_message_prefix()) and isn't affected by this bug.
  • New build_tool_context_thinking_visible_text() mirrors build_toolless_thinking_visible_text() but keeps the opening <think> tag, matching what chat_history_uses_tool_context() will actually render for this turn once it becomes history (the toolless helper strips it, which is correct only when tool context is absent).
  • New remember_tool_thinking_checkpoint() remembers prompt_text + build_tool_checkpoint_suffix(..., reasoning=NULL, ...) as the visible key after a tool-call finish, instead of unconditionally clearing it. A client that does replay real reasoning still gets an ordinary token-prefix hit first (before this fallback is ever consulted), so this is purely additive — it doesn't change behavior for clients that already work today.

Both new builders/functions are DeepSeek/DSML-syntax only (gated on model_syntax != SERVER_MODEL_SYNTAX_GLM); GLM keeps its current toolless-only behavior unchanged, since I haven't verified this mechanism against GLM's history-rendering shape and didn't want to guess at a model family I can't test.

Testing

  • Full existing ds4_test suite passes unchanged.
  • Two new regression tests: test_tool_context_thinking_visible_text_keeps_think_tag (verifies the two builders produce different shapes for the identical request, which is the whole point of not reusing the toolless one) and test_tool_checkpoint_suffix_omits_reasoning (verifies the remembered suffix omits reasoning while still reproducing the exact tool-call DSML).
  • Updated test_thinking_checkpoint_remember_gate to cover the new GLM/non-GLM split explicitly.

Happy to add a live A/B log-based repro (thinking live continuation match=visible-prefix vs token-mismatch) if useful, similar to what was posted on #611 — didn't include one here since the existing regression tests already pin down the exact byte-shape logic directly.

🤖 Generated with Claude Code

…turns

chat_history_uses_tool_context() makes render_deepseek_chat_prompt_text()
always keep the opening <think> tag for a historical assistant turn once any
tool schema is present -- "<think>{reasoning}</think>" -- rendering
{reasoning} from whatever reasoning_content the client echoes back for that
turn. Most OpenAI-compatible chat/completions clients never replay
reasoning_content, rendering an empty "<think></think>" there, while the live
KV session actually contains the real sampled reasoning bytes.

The existing visible-checkpoint mechanism already handles exactly this
mismatch for the toolless case (remember_thinking_checkpoint() /
should_remember_thinking_checkpoint()), but both are explicitly gated off
whenever request.has_tools is true, and the tool-call-finish path
(canonicalize_tool_checkpoint() or the raw-DSML-replay fallback) only ever
calls thinking_live_clear() afterwards -- so a tools-enabled conversation
never gets a live-continuation match once a non-replaying client's rendered
history diverges from the live tokens, which happens on essentially every
turn. This shows up as a full token-mismatch re-prefill on every turn in
issue antirez#609/antirez#611-adjacent workloads and is the root cause reported in antirez#691.

Fix, mirroring the toolless mechanism:
- should_remember_thinking_checkpoint() only excludes has_tools for GLM,
  whose history renderer keeps the opening <think> tag either way
  (append_glm_assistant_message_prefix()) and isn't affected by this bug.
- New build_tool_context_thinking_visible_text() mirrors
  build_toolless_thinking_visible_text() but keeps the opening <think> tag,
  matching what chat_history_uses_tool_context() will actually render for
  this turn once it becomes history.
- New remember_tool_thinking_checkpoint() remembers prompt_text +
  build_tool_checkpoint_suffix(..., reasoning=NULL, ...) as the visible key
  after a tool-call finish, instead of unconditionally clearing it. A client
  that *does* replay real reasoning still gets an ordinary token-prefix hit
  first, so this is purely additive.

Both new builders/functions are DeepSeek/DSML-syntax only; GLM keeps its
current (toolless-only) behavior unchanged, since it has a different history
rendering shape I have not verified against this mechanism.

Tested: full existing ds4_test suite passes, plus two new regression tests
(test_tool_context_thinking_visible_text_keeps_think_tag,
test_tool_checkpoint_suffix_omits_reasoning) and an updated
test_thinking_checkpoint_remember_gate covering the GLM/non-GLM split.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant