fix(tools): broad tool calling across every in-process engine

CHANGELOG.md

@@ -9,6 +9,100 @@ This project records release notes here and mirrors public-facing notes in
 
 ### Fixed
 
+- gpt-oss and DeepSeek V3.2 no longer return a tool the caller never offered.
+  Those two families parse their calls out of the token stream themselves and
+  are selected before the marker path, so the offered-tools rule never saw
+  them: a gpt-oss request sending `tool_choice: "none"`, which removes the
+  tools, still came back with a call, and its name carried the model's own
+  namespace prefix. Their output now passes through the same rule, and a
+  rejected call is delivered as content so the caller sees what the model did
+  rather than a blank answer.
+
+### Fixed
+
+- A model's parallel tool calls all reach the caller. Several families write
+  each call in its own block, and the stream consumer stops at the first chunk
+  carrying a finish reason, so one response per block delivered the first call
+  and dropped the rest. The calls of every block in a message are now coalesced
+  into a single response carrying a `tool_calls` array, which is the shape
+  OpenAI clients expect, and any text after the calls is released without a
+  finish reason so the tool response stays the terminal chunk.
+
+### Fixed
+
+- Reasoning no longer hides a tool call on the MLX engine. Tool parsing runs
+  downstream of the thinking parser, so a model that reasons before calling a
+  tool sent its reasoning through the tool parser first; that text decided the
+  message was not a call, and the marker that followed was never examined, so
+  the caller received the raw markup as content. Reasoning chunks now pass
+  straight through without taking part in that decision. This also means a call
+  a model only contemplated inside its reasoning is no longer executed, matching
+  the behavior the llama.cpp engine already had.
+
+### Fixed
+
+- `tool_choice` is now honored on the in-process engines. Only the served
+  engines forwarded it to a server that acts on it, so an MLX or llama.cpp
+  model ignored it entirely: a request sending `"none"` and asking for the tool
+  by name returned the tool call on every attempt. The option is now applied
+  before dispatch, so it means the same thing on every engine. `"none"` removes
+  the tools from the request, and naming a single function narrows the offered
+  tools to that one so the model cannot call a different tool than the caller
+  asked for. `"required"` remains a best-effort instruction on the in-process
+  engines, since forcing a call there would need constrained decoding.
+
+### Fixed
+
+- Tool calls whose markers arrive split across chunks are now recognized. A
+  generation chunk is whatever the streaming detokenizer could resolve that
+  step, not a token, so an opening marker that is a single token id still
+  reaches the parser in pieces: `<tool`, `_`, `c`, `all>`. The parser tested
+  each chunk on its own, so for most models the block never opened and the
+  caller received the raw markup as message content with a `stop` finish
+  reason. Observed on a Qwen model served by the MLX engine, where the model
+  emitted a perfectly well formed call. Text is now scanned across chunk
+  boundaries by carrying forward only the trailing run that could still become
+  a marker, and the closing marker is matched against the accumulated block.
+  That run is shorter than the longest marker, so ordinary answers stream with
+  at most a few characters of latency, and the scan keeps looking after
+  ordinary text has been released, so a model that writes a sentence before
+  calling ("I'll check that.") still has its call recognized. The unmarked
+  dialect opens on a brace, which also appears in prose, so there a call is
+  recognized only at the start of the message; its distinctive marker still
+  opens one anywhere. Text the model writes after closing a call is delivered
+  rather than swallowed into the block, and a second call in the same message
+  is recognized.
+
+### Fixed
+
+- Tool calling now works for Llama models on the MLX engine, and the shared
+  text parser recognizes the dialects the other families write. Llama declares
+  only its end-of-turn token as a stop token, not `<|eom_id|>`, which is how it
+  ends a message that hands off to a tool, so generation ran past the end of
+  the call and wrote the next turn's header into the answer text. Llama also
+  writes the call as a bare JSON object with no opening marker, so nothing
+  recognized it as a call at all: a caller offering a tool received JSON in
+  `content`, `finish_reason` of `stop`, and no `tool_calls`. Skulk now stops at
+  the message boundary for any model whose vocabulary has that token, and reads
+  the whole block with a set of cross-family dialects covering Llama
+  `<|python_tag|>` calls, Mistral `[TOOL_CALLS]` arrays, GLM
+  `<arg_key>`/`<arg_value>` pairs, and an unmarked call object that is the
+  entire message, alongside the harmony channels and `<tool_call>` blocks
+  already supported.
+
+- A model reaching for one of its own built-ins no longer surfaces as a tool
+  call. Llama answers some plain questions with a call to `print`, and gpt-oss
+  has `python` and `browser`; a caller has no implementation for those names,
+  so a response naming no offered tool is now returned as ordinary content. A
+  request that declares no tools is not parsed for calls at all, so a model
+  writing something call-shaped, which is what a request asking for JSON output
+  invites, cannot return `tool_calls` to a caller who offered none. Relatedly, text that opens
+  like a call but does not parse as one, which is what a model answering in
+  JSON looks like when tools are also offered, is returned as content instead
+  of being reported as a generation error.
+
+### Fixed
+
 - Chat completions never return an empty body, and streaming responses always
   terminate. A task that ended without producing any output, for example after
   being cancelled, previously tripped an assertion inside the response

resources/inference_model_cards/mlx-community--Qwen3.6-27B-4bit.toml

@@ -41,3 +41,9 @@ model_type = "qwen3_5"
 
 [storage_size]
 in_bytes = 16081490064
+
+# Tool calling is MODEL truth and does not vary by quantization: the sibling
+# cards for this base model declare it, so silence here would resolve to
+# "no tools" for the same model.
+[tooling]
+supports_tool_calling = true

resources/inference_model_cards/mlx-community--Qwen3.6-35B-A3B-nvfp4.toml

@@ -23,3 +23,9 @@ weights_repo = "mlx-community/Qwen3.6-35B-A3B-nvfp4"
 
 [storage_size]
 in_bytes = 20401929952
+
+# Tool calling is MODEL truth and does not vary by quantization: the sibling
+# cards for this base model declare it, so silence here would resolve to
+# "no tools" for the same model.
+[tooling]
+supports_tool_calling = true

src/skulk/api/adapters/chat_completions.py

@@ -4,7 +4,7 @@
 import re
 import time
 from collections.abc import AsyncGenerator
-from typing import Any
+from typing import Any, cast
 
 from loguru import logger
 
@@ -104,6 +104,60 @@ async def fetch_image_url(url: str) -> str:
         return base64.b64encode(data).decode("ascii")
 
 
+def resolve_tool_choice(
+    tools: list[dict[str, Any]] | None,
+    tool_choice: str | dict[str, Any] | None,
+) -> tuple[list[dict[str, Any]] | None, str | dict[str, Any] | None]:
+    """Apply ``tool_choice`` to the offered tools before dispatch.
+
+    Only the served engines forward ``tool_choice`` to a server that acts on
+    it. The in-process engines render whatever tools they are given and parse
+    whatever the model writes, so applying the caller's choice here is what
+    makes the option mean the same thing on every engine.
+
+    ``"none"`` removes the tools entirely, which is the only way to guarantee
+    the documented behavior that the model does not call one; a model handed a
+    tool and asked for it will call it whatever the request said. Naming a
+    single function narrows the offered tools to that one, so the model cannot
+    call a different tool than the caller asked for. ``"auto"``, ``"required"``
+    and an unrecognized value pass through untouched: ``required`` is a
+    best-effort instruction to the model in-process, since forcing a call would
+    need constrained decoding.
+
+    Returns the tools and the tool_choice to dispatch with.
+    """
+
+    if tool_choice is None or not tools:
+        return tools, tool_choice
+
+    if isinstance(tool_choice, str):
+        if tool_choice == "none":
+            # Dropping the choice with the tools keeps a served engine from
+            # being handed a tool_choice with nothing to choose from.
+            return None, None
+        return tools, tool_choice
+
+    function = tool_choice.get("function")
+    if not isinstance(function, dict):
+        return tools, tool_choice
+    name = cast("object", function.get("name"))  # pyright: ignore[reportUnknownMemberType]
+    if not isinstance(name, str):
+        return tools, tool_choice
+
+    named = [
+        tool
+        for tool in tools
+        if isinstance(tool.get("function"), dict)
+        and cast("dict[str, Any]", tool["function"]).get("name") == name
+    ]
+    # A name matching nothing is the caller's error. Emptying the list here
+    # would turn it into a silent prose answer, so the request is passed
+    # through whole: a served engine reports it, and an in-process engine
+    # answers from the full list. Rejecting it outright at this boundary is a
+    # follow-up, since only served engines report it today.
+    return (named or tools), tool_choice
+
+
 async def chat_request_to_text_generation(
     request: ChatCompletionRequest,
     *,
@@ -214,6 +268,14 @@ async def chat_request_to_text_generation(
         else request.top_logprobs is not None
     )
 
+    # Resolve tool_choice at the boundary for the same reason as logprobs: only
+    # the served engines forward it to a server that understands it, so an
+    # in-process engine would otherwise ignore it entirely and answer a "none"
+    # request with a tool call.
+    resolved_tools, resolved_tool_choice = resolve_tool_choice(
+        request.tools, request.tool_choice
+    )
+
     return TextGenerationTaskParams(
         model=request.model,
         input=input_messages
@@ -227,8 +289,8 @@ async def chat_request_to_text_generation(
         stop=request.stop,
         seed=request.seed,
         stream=request.stream,
-        tools=request.tools,
-        tool_choice=request.tool_choice,
+        tools=resolved_tools,
+        tool_choice=resolved_tool_choice,
         reasoning_effort=resolved_effort,
         enable_thinking=resolved_thinking,
         chat_template_messages=chat_template_messages

src/skulk/api/tests/test_tool_choice_resolution.py

@@ -0,0 +1,74 @@
+"""Coverage for applying ``tool_choice`` before dispatch.
+
+Only the served engines forward ``tool_choice`` to a server that acts on it, so
+without this resolution an in-process engine answers a ``"none"`` request with
+a tool call. That was observed live: a Llama model on the MLX engine returned
+`get_weather` on all four attempts of a `"none"` request that asked for the
+tool by name.
+"""
+
+from __future__ import annotations
+
+from typing import Any
+
+from skulk.api.adapters.chat_completions import resolve_tool_choice
+
+WEATHER: dict[str, Any] = {
+    "type": "function",
+    "function": {"name": "get_weather", "parameters": {"type": "object"}},
+}
+TIME: dict[str, Any] = {
+    "type": "function",
+    "function": {"name": "get_time", "parameters": {"type": "object"}},
+}
+BOTH = [WEATHER, TIME]
+
+
+def names(tools: list[dict[str, Any]] | None) -> list[str]:
+    return [] if tools is None else [tool["function"]["name"] for tool in tools]
+
+
+class TestNone:
+    def test_none_removes_the_tools_entirely(self) -> None:
+        tools, choice = resolve_tool_choice(BOTH, "none")
+        assert tools is None
+        assert choice is None
+
+    def test_none_without_tools_is_a_no_op(self) -> None:
+        assert resolve_tool_choice(None, "none") == (None, "none")
+
+
+class TestNamedFunction:
+    def test_a_named_function_narrows_the_offered_tools(self) -> None:
+        tools, choice = resolve_tool_choice(
+            BOTH, {"type": "function", "function": {"name": "get_time"}}
+        )
+        assert names(tools) == ["get_time"]
+        # The choice still travels, so a served engine enforces it server-side.
+        assert choice == {"type": "function", "function": {"name": "get_time"}}
+
+    def test_a_name_matching_nothing_is_left_for_the_engine_to_report(self) -> None:
+        # Silently sending no tools would turn the caller's mistake into a
+        # confusing prose answer instead of an error.
+        tools, _ = resolve_tool_choice(
+            BOTH, {"type": "function", "function": {"name": "nope"}}
+        )
+        assert names(tools) == ["get_weather", "get_time"]
+
+    def test_a_malformed_choice_object_passes_through(self) -> None:
+        tools, choice = resolve_tool_choice(BOTH, {"type": "function"})
+        assert names(tools) == ["get_weather", "get_time"]
+        assert choice == {"type": "function"}
+
+
+class TestPassThrough:
+    def test_auto_is_untouched(self) -> None:
+        assert resolve_tool_choice(BOTH, "auto") == (BOTH, "auto")
+
+    def test_required_is_untouched(self) -> None:
+        # In-process there is no constrained decoding to force a call, so this
+        # stays a best-effort instruction rather than being reinterpreted here.
+        assert resolve_tool_choice(BOTH, "required") == (BOTH, "required")
+
+    def test_an_absent_choice_is_untouched(self) -> None:
+        assert resolve_tool_choice(BOTH, None) == (BOTH, None)

src/skulk/worker/engines/mlx/utils_mlx.py

@@ -1357,6 +1357,39 @@ def _patched_encode(text: str, **_kwargs: object) -> list[int]:
         else:
             tokenizer.eos_token_ids = [gemma_eos_id, gemma_end_of_turn_id]
 
+    # Llama 3.1+ ends a tool-calling turn with <|eom_id|> ("end of message",
+    # handing off to a tool) and a user-facing turn with <|eot_id|> ("end of
+    # turn"). Only <|eot_id|> reaches us from tokenizer_config, because
+    # generation_config carries no eos_token_id for these repos, so without
+    # this the model runs straight past the end of its own tool call: the
+    # scaffolding detokenizes into visible content and a second call begins.
+    # Upstream (Meta's reference, vLLM and llama.cpp) all stop on both.
+    # Detected by vocabulary rather than by the template mentioning the token:
+    # Llama 3.2's template never writes <|eom_id|> or <|python_tag|> literally,
+    # it only routes tool results through the "ipython" role, so a template
+    # substring check silently misses the family this exists for.
+    llama_eom_id = _token_id_or_none(tokenizer, "<|eom_id|>")
+    if llama_eom_id is not None:
+        # <|eom_id|> is "end of message, handing off to a tool". Llama declares
+        # only <|eot_id|> as its stop token, so without this the model runs
+        # straight past the end of its tool call and generates the next turn's
+        # header, and the caller sees control tokens in the answer text.
+        existing = list(tokenizer.eos_token_ids or [])
+        if llama_eom_id not in existing:
+            tokenizer.eos_token_ids = existing + [llama_eom_id]
+        if not getattr(tokenizer, "tool_parser", None):
+            # Llama writes the call as a bare object with no opening marker, so
+            # the block opens on "{" and is closed by the end of the message
+            # rather than by a closing marker. The whole-block dialect parser
+            # reads both that form and the <|python_tag|> variant.
+            object.__setattr__(tokenizer, "_tool_call_start", "{")
+            object.__setattr__(tokenizer, "_tool_call_end", "<|eom_id|>")
+            from skulk.worker.runner.llm_inference.tool_parsers import (
+                UNMARKED_TOOL_DIALECT,
+            )
+
+            object.__setattr__(tokenizer, "_tool_parser", UNMARKED_TOOL_DIALECT)
+
     if capability_profile.tool_call_format == ToolCallFormat.Gemma4:
         # mlx-lm exposes tool-call markers through read-only properties on
         # TokenizerWrapper. Configure the internal fields directly so Gemma 4
@@ -1778,6 +1811,28 @@ def mx_barrier(group: Group | None):
     )
 
 
+def _token_id_or_none(tokenizer: object, token: str) -> int | None:
+    """Return a special token's id, or None when the tokenizer lacks it.
+
+    Vocabularies differ across quantizations and conversions, so a missing
+    token is normal and must not raise.
+    """
+
+    convert = getattr(tokenizer, "convert_tokens_to_ids", None)
+    if convert is None:
+        return None
+    try:
+        token_id = cast("object", convert(token))
+    except Exception:  # noqa: BLE001 - tokenizer implementations vary
+        return None
+    if not isinstance(token_id, int):
+        return None
+    unknown = getattr(tokenizer, "unk_token_id", None)
+    if token_id < 0 or (unknown is not None and token_id == unknown):
+        return None
+    return token_id
+
+
 def _parse_generic_text_tool_calls(text: str) -> list[dict[str, Any]]:
     """Parse generic-format tool calls (Qwen3 XML or Hermes JSON) from text.