diff --git a/CHANGELOG.md b/CHANGELOG.md index 6f3894361..3ff063ea7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,116 @@ This project records release notes here and mirrors public-facing notes in ### Fixed +- A request offering no tools still has its markers stripped. Skipping the scan + entirely when none were offered, which is what keeps `tool_choice: "none"` + from producing a call, also meant nothing recognized a block the model wrote + anyway, so its markers went straight to the caller. The block is now always + recognized; whether it may become a call is what depends on the request. + +- A tool call handed back as content no longer carries the model's control + tokens. When a call names no offered tool it is delivered as text so the + caller can see what the model did, but the block was handed back verbatim, so + `<|python_tag|>` and `` markers ended up in the answer. The + markers are stripped from an answer; a response already flagged as an error + still carries the raw block, since there it is the evidence of what was + malformed. + +### 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: ``. 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 + ``/`` pairs, and an unmarked call object that is the + entire message, alongside the harmony channels and `` 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 diff --git a/resources/inference_model_cards/mlx-community--Qwen3.6-27B-4bit.toml b/resources/inference_model_cards/mlx-community--Qwen3.6-27B-4bit.toml index 15926d8bc..028a6930c 100644 --- a/resources/inference_model_cards/mlx-community--Qwen3.6-27B-4bit.toml +++ b/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 diff --git a/resources/inference_model_cards/mlx-community--Qwen3.6-35B-A3B-nvfp4.toml b/resources/inference_model_cards/mlx-community--Qwen3.6-35B-A3B-nvfp4.toml index a785cd172..72860b9a3 100644 --- a/resources/inference_model_cards/mlx-community--Qwen3.6-35B-A3B-nvfp4.toml +++ b/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 diff --git a/src/skulk/api/adapters/chat_completions.py b/src/skulk/api/adapters/chat_completions.py index a3bf56ef6..a76ce9f1e 100644 --- a/src/skulk/api/adapters/chat_completions.py +++ b/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 diff --git a/src/skulk/api/tests/test_tool_choice_resolution.py b/src/skulk/api/tests/test_tool_choice_resolution.py new file mode 100644 index 000000000..b1934cf5a --- /dev/null +++ b/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) diff --git a/src/skulk/shared/models/tests/test_bundled_card_tooling_agreement.py b/src/skulk/shared/models/tests/test_bundled_card_tooling_agreement.py new file mode 100644 index 000000000..b6a94710f --- /dev/null +++ b/src/skulk/shared/models/tests/test_bundled_card_tooling_agreement.py @@ -0,0 +1,125 @@ +"""Bundled cards for one base model must not contradict each other on tools. + +Whether a model can call tools is a property of the model, so two cards for the +same `base_model` cannot both be right when one says it can and the other says +it cannot. A contradiction is not academic: it decides whether the API +advertises the capability, and on the served engines whether a request carrying +tools is rejected outright. It also reaches callers through `/v1/models`, so a +client picking a quantization can be told the same model does and does not +support tools depending which one it picked. + +An unstated value is not a contradiction. A card with no `[tooling]` section +resolves through conservative family defaults, so silence next to an explicit +claim is under-specification rather than disagreement, and that is not what +this guards. +""" + +from __future__ import annotations + +import tomllib +from collections import defaultdict +from pathlib import Path +from typing import Any, cast + +CARD_DIRECTORY = ( + Path(__file__).resolve().parents[5] / "resources" / "inference_model_cards" +) + +# Contradictions that exist today and are tracked for the signed registry +# rather than fixed here. The vLLM-only cards under-declare: those models do +# call tools, but the vLLM runner resolves a parser only from an explicit +# `runtime.vllm_tool_call_parser` pin and rejects a tools request without one, +# so flipping the flag alone would advertise a capability that fails at request +# time. Pinning a parser has to be validated on GPU hardware first. Anything +# NOT listed here is a new contradiction and fails. +KNOWN_CONTRADICTIONS: frozenset[str] = frozenset( + { + "Qwen3.6 27B", + "Qwen3.6 35B A3B", + } +) + + +def load_cards() -> dict[str, dict[str, Any]]: + """Return every bundled card keyed by file name.""" + + cards: dict[str, dict[str, Any]] = {} + for path in sorted(CARD_DIRECTORY.glob("*.toml")): + cards[path.name] = tomllib.loads(path.read_text()) + return cards + + +def group_by_base_model() -> dict[str, list[tuple[str, dict[str, Any]]]]: + """Group bundled cards by their declared base model.""" + + grouped: dict[str, list[tuple[str, dict[str, Any]]]] = defaultdict(list) + for name, card in load_cards().items(): + base = card.get("base_model") + if isinstance(base, str) and base: + grouped[base].append((name, card)) + return grouped + + +def tooling_of(card: dict[str, Any]) -> dict[str, Any]: + section = cast("object", card.get("tooling")) + if not isinstance(section, dict): + return {} + return cast("dict[str, Any]", section) + + +class TestToolingAgreement: + def test_the_card_directory_was_found(self) -> None: + # A wrong path would make every assertion below vacuous. + assert CARD_DIRECTORY.is_dir() + assert len(load_cards()) > 50 + + def test_no_two_cards_disagree_on_whether_tools_are_supported(self) -> None: + offenders: dict[str, dict[bool, list[str]]] = {} + for base, group in group_by_base_model().items(): + stated: dict[bool, list[str]] = defaultdict(list) + for name, card in group: + supports = tooling_of(card).get("supports_tool_calling") + if isinstance(supports, bool): + stated[supports].append(name) + if len(stated) > 1 and base not in KNOWN_CONTRADICTIONS: + offenders[base] = dict(stated) + assert not offenders, ( + "cards for one base model disagree on tool support: " + f"{offenders}. Whether a model can call tools is a property of the " + "model, so one of these is wrong." + ) + + def test_no_two_cards_disagree_on_the_tool_call_format(self) -> None: + offenders: dict[str, dict[str, list[str]]] = {} + for base, group in group_by_base_model().items(): + stated: dict[str, list[str]] = defaultdict(list) + for name, card in group: + fmt = tooling_of(card).get("tool_call_format") + if isinstance(fmt, str): + stated[fmt].append(name) + if len(stated) > 1 and base not in KNOWN_CONTRADICTIONS: + offenders[base] = dict(stated) + assert not offenders, ( + "cards for one base model disagree on the tool-call dialect: " + f"{offenders}. The dialect a model writes is a property of the " + "model, and wiring the wrong markers makes its calls unparseable." + ) + + def test_every_known_contradiction_still_exists(self) -> None: + # The exception list is debt, not decoration: once the registry fixes + # one, this fails so the entry is removed rather than quietly widening + # what the guard above allows. + grouped = group_by_base_model() + still_wrong: set[str] = set() + for base in KNOWN_CONTRADICTIONS: + stated: set[bool] = set() + for _, card in grouped.get(base, []): + supports = tooling_of(card).get("supports_tool_calling") + if isinstance(supports, bool): + stated.add(supports) + if len(stated) > 1: + still_wrong.add(base) + assert still_wrong == set(KNOWN_CONTRADICTIONS), ( + "KNOWN_CONTRADICTIONS is stale; remove the entries that now agree: " + f"{set(KNOWN_CONTRADICTIONS) - still_wrong}" + ) diff --git a/src/skulk/worker/engines/mlx/utils_mlx.py b/src/skulk/worker/engines/mlx/utils_mlx.py index bdf9e641d..6ece235a5 100644 --- a/src/skulk/worker/engines/mlx/utils_mlx.py +++ b/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. diff --git a/src/skulk/worker/runner/llama_cpp/runner.py b/src/skulk/worker/runner/llama_cpp/runner.py index 83b6b495f..8a5115358 100644 --- a/src/skulk/worker/runner/llama_cpp/runner.py +++ b/src/skulk/worker/runner/llama_cpp/runner.py @@ -17,6 +17,7 @@ """ import inspect +import json import os import time from collections.abc import Callable @@ -64,6 +65,7 @@ ) from skulk.worker.runner.llm_inference.harmony_text_parser import HarmonyTextParser from skulk.worker.runner.llm_inference.think_text_parser import ThinkTextParser +from skulk.worker.runner.llm_inference.tool_parsers import declared_tool_calls from skulk.worker.runner.llm_inference.tool_text_parser import ( parse_tool_calls_from_text, ) @@ -450,6 +452,41 @@ def tool_calls_from_message(message: dict[str, Any]) -> list[ToolCallItem]: return items +def dropped_call_text(message: dict[str, Any]) -> str: + """Render native calls as text, for calls that named no offered tool. + + When llama.cpp's own handler parses a call, the raw markup is gone from the + message and ``content`` is null. Dropping such a call without putting + anything in its place would answer the request with a successful blank + message, so the call is re-serialized and delivered as content, which is + what the text-recovered path does with a block naming no offered tool. + """ + + rendered = [ + json.dumps({"name": call.name, "arguments": call.arguments}) + for call in tool_calls_from_message(message) + ] + return "\n".join(rendered) + + +def offered_tool_calls_from_message( + message: dict[str, Any], tools: list[dict[str, Any]] | None +) -> list[ToolCallItem]: + """Native structured calls from llama.cpp, limited to the offered tools. + + llama.cpp's bundled chat handlers fill ``tool_calls`` themselves for the + formats they recognize, and nothing there checks the name against the + request. A model reaching for one of its own built-ins would otherwise + reach the caller as a call they cannot run, which is the same rule the + text-recovered path applies, and a request that offered no tools cannot + produce one at all. + """ + + if not tools: + return [] + return declared_tool_calls(tool_calls_from_message(message), tools) + + def _logprob_fields( choice: dict[str, Any], ) -> tuple[float | None, list[TopLogprobItem] | None]: @@ -1265,8 +1302,20 @@ def _generate_with_tools( ) visible_text = "".join(text for text, is_thinking in emissions if not is_thinking) - tool_calls = tool_calls_from_message(message) - if not tool_calls: + tool_calls = offered_tool_calls_from_message(message, task.task_params.tools) + if not tool_calls and not visible_text.strip(): + # The handler consumed the raw markup while parsing, so a call that + # named no offered tool leaves nothing to say. Put the call back as + # text rather than answering with a successful blank message. + restored = dropped_call_text(message) + if restored: + visible_text = restored + emissions = emissions + [(restored, False)] + # The plain-model branch below emits `content`, which the + # handler emptied when it parsed the call, so it needs the + # restored text too or the answer is still blank. + content = restored + if not tool_calls and task.task_params.tools: # llama.cpp only fills structured tool_calls for formats its bundled # chat handlers recognize. A reasoning model emits the call as text, # so recover it from the string (#416). Source selection matters: diff --git a/src/skulk/worker/runner/llama_cpp/tests/test_llama_cpp_helpers.py b/src/skulk/worker/runner/llama_cpp/tests/test_llama_cpp_helpers.py index 506b68b73..f2b028045 100644 --- a/src/skulk/worker/runner/llama_cpp/tests/test_llama_cpp_helpers.py +++ b/src/skulk/worker/runner/llama_cpp/tests/test_llama_cpp_helpers.py @@ -2,6 +2,7 @@ """Tests for the pure helpers of the llama.cpp runner (no llama_cpp needed).""" from pathlib import Path +from typing import Any import pytest @@ -18,11 +19,13 @@ _logprob_fields, _sanitize_harmony_assistant_messages, _splice_images_into_messages, + dropped_call_text, find_mmproj_file, generation_kwargs, logprobs_unavailable_error, map_finish_reason, messages_for_llama, + offered_tool_calls_from_message, select_gguf_file, serving_n_ctx, tool_calls_from_message, @@ -424,3 +427,56 @@ def test_vision_handler_map_defaults_to_mtmd() -> None: assert _VISION_HANDLER_BY_MODEL_TYPE["qwen2.5-vl"] == "Qwen25VLChatHandler" assert _VISION_HANDLER_BY_MODEL_TYPE.get("some-new-vlm") is None assert _DEFAULT_VISION_HANDLER == "MTMDChatHandler" + + +WEATHER_TOOL: dict[str, Any] = { + "type": "function", + "function": {"name": "get_weather", "parameters": {"type": "object"}}, +} +NATIVE_WEATHER_CALL: dict[str, Any] = { + "tool_calls": [ + { + "id": "call_1", + "type": "function", + "function": {"name": "get_weather", "arguments": "{}"}, + } + ] +} +NATIVE_BUILTIN_CALL: dict[str, Any] = { + "tool_calls": [ + { + "id": "call_1", + "type": "function", + "function": {"name": "python", "arguments": "{}"}, + } + ] +} + + +def test_offered_tool_calls_keeps_a_call_to_an_offered_tool() -> None: + items = offered_tool_calls_from_message(NATIVE_WEATHER_CALL, [WEATHER_TOOL]) + assert [item.name for item in items] == ["get_weather"] + + +def test_offered_tool_calls_drops_a_call_to_a_tool_nobody_offered() -> None: + # llama.cpp's bundled chat handlers fill tool_calls themselves and nothing + # there checks the name against the request, so a model reaching for one of + # its own built-ins would otherwise reach the caller as a call to run. + assert offered_tool_calls_from_message(NATIVE_BUILTIN_CALL, [WEATHER_TOOL]) == [] + + +def test_offered_tool_calls_returns_nothing_when_no_tools_were_offered() -> None: + assert offered_tool_calls_from_message(NATIVE_WEATHER_CALL, None) == [] + assert offered_tool_calls_from_message(NATIVE_WEATHER_CALL, []) == [] + + +def test_dropped_call_text_renders_the_call_so_the_answer_is_not_blank() -> None: + # The handler consumed the raw markup while parsing, so dropping the call + # without putting anything in its place would answer a request with a + # successful blank message. + text = dropped_call_text(NATIVE_BUILTIN_CALL) + assert "python" in text + + +def test_dropped_call_text_is_empty_when_there_was_no_call() -> None: + assert dropped_call_text({"content": "hi"}) == "" diff --git a/src/skulk/worker/runner/llm_inference/model_output_parsers.py b/src/skulk/worker/runner/llm_inference/model_output_parsers.py index 0d4311f9b..d9be58bd8 100644 --- a/src/skulk/worker/runner/llm_inference/model_output_parsers.py +++ b/src/skulk/worker/runner/llm_inference/model_output_parsers.py @@ -1,3 +1,4 @@ +import json from collections.abc import Generator from functools import cache from typing import Any @@ -32,7 +33,10 @@ detect_thinking_prompt_suffix, ) from skulk.worker.runner.bootstrap import logger -from skulk.worker.runner.llm_inference.tool_parsers import ToolParser +from skulk.worker.runner.llm_inference.tool_parsers import ( + ToolParser, + declared_tool_calls, +) _GEMMA4_THINK_START = "<|channel>thought\n" _GEMMA4_THINK_END = "" @@ -128,16 +132,28 @@ def apply_all_parsers( if capability_profile.output_parser == OutputParserType.GptOss or issubclass( model_type, GptOssModel ): - mlx_generator = parse_gpt_oss(mlx_generator) + # These two parse their own calls out of the token stream, so unlike + # the marker path they need the offered-tools rule applied downstream. + mlx_generator = reject_unoffered_tool_calls( + parse_gpt_oss(mlx_generator), tools + ) elif capability_profile.output_parser == OutputParserType.DeepseekV32 or issubclass( model_type, DeepseekV32Model ): - mlx_generator = parse_deepseek_v32(mlx_generator) + mlx_generator = reject_unoffered_tool_calls( + parse_deepseek_v32(mlx_generator), tools + ) elif tool_parser: + # Always scan, even with no tools offered. The parser is wired from the + # tokenizer and cannot see the request, so `emit_calls` carries that: + # with no tools nothing may be returned as a call, which is what makes + # tool_choice "none" hold, but the block is still recognized so its + # markers are stripped rather than delivered to the caller. mlx_generator = parse_tool_calls( mlx_generator, tool_parser, tools, + emit_calls=bool(tools), trace_task_id=trace_task_id, trace_rank=trace_rank, ) @@ -677,16 +693,267 @@ def _emit_text( ) +def reject_unoffered_tool_calls( + responses: Generator[ParserChunk], tools: list[dict[str, Any]] | None +) -> Generator[ParserChunk]: + """Keep a family parser from returning a tool the caller never offered. + + gpt-oss and DeepSeek V3.2 parse their calls from the token stream + themselves, so they never pass through the offered-tools filter the marker + path applies. Observed live on gpt-oss: a request sending + ``tool_choice: "none"``, which removes the tools, still came back with a + call, and its name carried the harmony namespace prefix as well. + + The rejected call is delivered as content, which is what every other path + here does with a block naming no offered tool, so the caller sees what the + model did rather than an empty answer. + """ + + template: GenerationResponse | None = None + pending_text = "" + rejected: ToolCallResponse | None = None + for response in responses: + if response is None: + yield None + continue + if isinstance(response, ToolCallResponse): + kept = declared_tool_calls(response.tool_calls, tools) if tools else [] + if kept: + yield response.model_copy(update={"tool_calls": kept}) + continue + # Held rather than emitted with a finish reason of its own: these + # streams usually carry a terminal chunk after the call, and adding + # a second terminal would end the stream at the consumer before the + # real one arrives. If none follows, it is released at the end. + rendered = [ + json.dumps({"name": call.name, "arguments": call.arguments}) + for call in response.tool_calls + ] + # Separated, so several rejected calls in a row do not run together + # into text a caller cannot read back. + pending_text = "\n".join( + part for part in [pending_text, *rendered] if part + ) + rejected = response + continue + template = response + if pending_text: + yield response.model_copy( + update={ + "text": pending_text + response.text, + "token": 0, + "is_thinking": False, + } + ) + pending_text = "" + continue + yield response + if pending_text: + # The rejected response's accounting is the message's accounting, so it + # is carried rather than replaced with a fabricated empty one. + base = template or GenerationResponse(text="", token=0, usage=None) + yield base.model_copy( + update={ + "text": pending_text, + "token": 0, + "is_thinking": False, + "finish_reason": "stop", + "usage": rejected.usage if rejected is not None else base.usage, + "stats": rejected.stats if rejected is not None else base.stats, + } + ) + + +def _block_as_content(text: str, tool_parser: ToolParser) -> str: + """Strip a dialect's markers from a block being delivered as content. + + A block that named no offered tool, or that did not parse but reads as an + answer, is handed back to the caller as content. Handing it back verbatim + puts the dialect's control tokens in their answer text, which is the leak + this whole path exists to prevent: `<|python_tag|>` reached a caller that + way, found by the harness's tool-contract suite. + + The error path deliberately does NOT use this. There the raw block is the + evidence of what was malformed, and the response is already flagged as an + error rather than offered as an answer. + """ + + stripped = text + for marker in (*tool_parser.start_markers, tool_parser.end_parsing): + if marker and marker != "{": + stripped = stripped.replace(marker, "") + return stripped + + +def _block_start_index( + text: str, tool_parser: ToolParser, *, at_message_start: bool +) -> int | None: + """Index in ``text`` where a tool-call block begins, or ``None``. + + Distinctive markers open a block wherever they appear, because models + routinely write a sentence before calling ("I'll check that." then the + call). The unmarked dialect's opening marker is ``{``, which appears in + ordinary prose and JSON answers, so it opens a block only at the start of + the message, which is the only place the families using it write a call. + """ + + earliest: int | None = None + for marker in tool_parser.extra_start_parsing: + found = text.find(marker) + if found != -1 and (earliest is None or found < earliest): + earliest = found + + if not tool_parser.anchored: + found = text.find(tool_parser.start_parsing) + if found != -1 and (earliest is None or found < earliest): + earliest = found + elif at_message_start: + stripped = text.lstrip() + if stripped.startswith(tool_parser.start_parsing): + found = len(text) - len(stripped) + if earliest is None or found < earliest: + earliest = found + return earliest + + +def _partial_marker_suffix_length(text: str, markers: tuple[str, ...]) -> int: + """Length of the trailing run of ``text`` that could still become a marker. + + Held back rather than emitted, so a marker split across chunks is still + recognized. Bounded by the longest marker, so this is a few characters of + latency at most and never an unbounded buffer. + """ + + longest = max(len(marker) for marker in markers) - 1 + for length in range(min(longest, len(text)), 0, -1): + tail = text[-length:] + if any(marker.startswith(tail) for marker in markers): + return length + return 0 + + +def _scan_remaining_blocks( + text: str, tool_parser: ToolParser, tools: list[dict[str, Any]] | None +) -> tuple[list[ToolCallItem], str]: + """Parse every remaining block in a complete text. + + Used once generation has ended, where there is no further chunk to drive + the streaming scan and the rest of the message is already in hand. Returns + the calls found and the text that was not part of any block, so a message + that puts a call the caller cannot run before one they can still delivers + the second call and the surrounding prose. + """ + + calls: list[ToolCallItem] = [] + leftover: list[str] = [] + remaining = text + while remaining: + start = _block_start_index(remaining, tool_parser, at_message_start=False) + if start is None: + leftover.append(remaining) + break + end = remaining.find(tool_parser.end_parsing, start) + if end == -1: + leftover.append(remaining) + break + end_of_block = end + len(tool_parser.end_parsing) + block = remaining[start:end_of_block] + parsed = tool_parser.parse(block.strip(), tools=tools) + kept = declared_tool_calls(parsed, tools) if parsed is not None else [] + if kept: + leftover.append(remaining[:start]) + calls.extend(kept) + else: + # Not a call the caller can run, so it is text like any other. + leftover.append(remaining[:end_of_block]) + remaining = remaining[end_of_block:] + return calls, "".join(leftover) + + def parse_tool_calls( responses: Generator[ParserChunk], tool_parser: ToolParser, tools: list[dict[str, Any]] | None, *, + emit_calls: bool = True, trace_task_id: str | None = None, trace_rank: int = 0, ) -> Generator[ParserChunk]: + """Recover tool calls from the generated stream, one response per message. + + The calls of every block in a message are coalesced into a single + ``ToolCallResponse``. That is the OpenAI shape, where one assistant message + carries a ``tool_calls`` array, and it is what makes a model's parallel + calls survive: several families write each call in its own block, and the + consumer of this stream stops at the first chunk carrying a finish reason, + so a response per block would deliver the first call and drop the rest. + """ + in_tool_call = False + # Held until the message ends rather than emitted per block, so several + # blocks arrive as one response. The stream does not end when generation + # does (the source keeps idling), so the terminal chunk is the signal. + accumulated_calls: list[ToolCallItem] = [] + last_response: GenerationResponse | None = None tool_call_text_parts: list[str] = [] + # A chunk is whatever the streaming detokenizer could resolve this step, not + # a token: an opening marker that is one token id still arrives split across + # chunks (""). Testing each chunk on its own misses + # the marker for most models, so text is scanned across chunk boundaries by + # carrying forward only the trailing run that could still become a marker. + # That run is shorter than the longest marker, so ordinary answers stream + # with at most a few characters of latency and nothing is ever held for a + # message that turns out not to contain a call. + held_text = "" + at_message_start = True + def _finish_message( + response: GenerationResponse, + ) -> Generator[ParserChunk]: + """Close out a message once a block has been dealt with. + + Every exit from the close site routes through here so the same three + rules hold whatever the block turned out to be: the rest of the message + is parsed rather than emitted whole (no further chunk will arrive to + drive the streaming scan), the calls found across the whole message are + delivered as one response, and exactly one chunk carries the finish + reason, since the consumer stops at the first one that does. + """ + + nonlocal held_text, accumulated_calls + if response.finish_reason is None: + return + terminal_sent = False + if held_text: + more_calls, leftover = _scan_remaining_blocks( + held_text, tool_parser, tools + ) + accumulated_calls.extend(more_calls) + held_text = "" + if leftover: + carries_finish = not accumulated_calls + yield response.model_copy( + update={ + "text": leftover, + "token": 0, + "finish_reason": response.finish_reason + if carries_finish + else None, + } + ) + terminal_sent = carries_finish + if accumulated_calls: + yield ToolCallResponse( + tool_calls=accumulated_calls, + usage=response.usage, + stats=response.stats, + ) + accumulated_calls = [] + return + if not terminal_sent: + # The block's own content went out without the finish reason, so + # something still has to end the stream. + yield response.model_copy(update={"text": "", "token": 0}) + for response in responses: if response is None: yield None @@ -695,18 +962,118 @@ def parse_tool_calls( yield response continue - if not in_tool_call and response.text.startswith(tool_parser.start_parsing): - in_tool_call = True - - if not in_tool_call: + # Reasoning is never part of a tool-call block: this parser runs + # downstream of the thinking parser, and a call a model only + # contemplated inside its reasoning must not be executed. Passing those + # chunks straight through also keeps them out of the opening decision, + # so a thinking model that reasons before calling still has its marker + # examined when the visible answer begins. + if response.is_thinking: yield response continue - tool_call_text_parts.append(response.text) - if response.text.endswith(tool_parser.end_parsing): - # parse the actual tool calls from the tool call text - combined = "".join(tool_call_text_parts) + last_response = response + just_opened = False + if not in_tool_call: + scanned = held_text + response.text + start = _block_start_index( + scanned, tool_parser, at_message_start=at_message_start + ) + if start is not None: + preamble = scanned[:start] + if preamble: + yield response.model_copy( + update={ + "text": preamble, + "token": 0, + "finish_reason": None, + } + ) + in_tool_call = True + just_opened = True + held_text = "" + at_message_start = False + tool_call_text_parts.append(scanned[start:]) + else: + keep = _partial_marker_suffix_length( + scanned, tool_parser.start_markers + ) + if response.finish_reason is not None: + # Nothing more is coming, so a partial marker is just text. + keep = 0 + emitted = scanned[: len(scanned) - keep] + held_text = scanned[len(scanned) - keep :] + if emitted.strip(): + at_message_start = False + if response.finish_reason is not None and accumulated_calls: + # A call was found earlier in this message and the tool + # response has to be the terminal chunk, so this trailing + # text is released without the finish reason. + if emitted: + yield response.model_copy( + update={ + "text": emitted, + "token": 0, + "finish_reason": None, + } + ) + yield ToolCallResponse( + tool_calls=accumulated_calls, + usage=response.usage, + stats=response.stats, + ) + accumulated_calls = [] + continue + if emitted == response.text and not held_text: + yield response + elif emitted or response.finish_reason is not None: + yield response.model_copy( + update={"text": emitted, "token": 0} + ) + continue + + if not just_opened: + tool_call_text_parts.append(response.text) + # The closing marker splits across chunks for the same reason the + # opening one does, so it is located in the accumulated block rather + # than tested against one chunk. Locating rather than matching the end + # also matters because a model may keep writing after the call ("... + # Done."): everything past the marker is ordinary text and + # goes back to the opening scan, where a second call in the same + # message is still found. + block_so_far = "".join(tool_call_text_parts) + end_index = block_so_far.find(tool_parser.end_parsing) + if end_index != -1: + end_of_block = end_index + len(tool_parser.end_parsing) + combined = block_so_far[:end_of_block] + held_text = block_so_far[end_of_block:] + tool_call_text_parts = [combined] parsed = tool_parser.parse(combined.strip(), tools=tools) + if parsed is not None: + # With no tools offered nothing may be called, but the block + # still has to be recognized: skipping the scan entirely left + # the markers in the answer, which is what a caller saw. + kept = declared_tool_calls(parsed, tools) if emit_calls else [] + if not kept: + logger.info( + "Block named no offered tool, emitting it as content " + f"(parsed_calls={len(parsed)})" + ) + in_tool_call = False + tool_call_text_parts = [] + # The remainder stays with the scan rather than being + # emitted here, so a further call in the trailing text is + # still found. + yield response.model_copy( + update={ + "text": _block_as_content(combined, tool_parser), + "token": 0, + "finish_reason": None, + } + ) + yield from _finish_message(response) + continue + parsed = kept logger.info( "Parsed generated tool-call block " f"(chunks={len(tool_call_text_parts)}, " @@ -716,6 +1083,21 @@ def parse_tool_calls( in_tool_call = False tool_call_text_parts = [] + if parsed is None and tool_parser.unparsed_is_text: + logger.info( + "Unmarked block did not parse as a tool call, " + f"emitting it as content (generated_chars={len(combined)})" + ) + yield response.model_copy( + update={ + "text": _block_as_content(combined, tool_parser), + "token": 0, + "finish_reason": None, + } + ) + yield from _finish_message(response) + continue + if parsed is None: logger.warning( "Tool-call parsing failed " @@ -744,20 +1126,107 @@ def parse_tool_calls( tags=["tool_call"], attrs={"tool_call_count": len(parsed)}, ) - yield ToolCallResponse( - tool_calls=parsed, usage=response.usage, stats=response.stats - ) + accumulated_calls.extend(parsed) + yield from _finish_message(response) continue if response.finish_reason is not None: + # Generation ended while inside a tool-call block. That is not + # always truncation: several families close a call by ending the + # message rather than by emitting a closing marker. Llama 3.1+ is + # the clearest case, where <|eom_id|> means "end of message, + # handing off to a tool", so the block is complete and the closing + # marker never arrives. Try to parse before declaring it garbage; + # only a block that genuinely does not parse falls through to the + # error path, which is what truncation actually looks like. + combined = "".join(tool_call_text_parts) + # Truncation is the one case where an unclosed block must not be + # read as a call. A marker dialect's inner parser only strips the + # closing marker if it is there, so a call cut off at max_tokens + # would otherwise parse and be handed to the caller to execute. + parsed = ( + None + if response.finish_reason == "length" + else tool_parser.parse(combined.strip(), tools=tools) + ) + if parsed is not None and ( + not emit_calls or not declared_tool_calls(parsed, tools) + ): + logger.info( + "Block named no offered tool, emitting it as content " + f"(parsed_calls={len(parsed)})" + ) + yield response.model_copy( + update={ + "text": _block_as_content(combined, tool_parser), + "token": 0, + "finish_reason": None, + } + ) + yield from _finish_message(response) + break + if parsed and emit_calls: + parsed = declared_tool_calls(parsed, tools) + logger.info( + "Parsed tool-call block closed by end of generation " + f"(generated_chars={len(combined)}, parsed_calls={len(parsed)})" + ) + if trace_task_id is not None: + record_trace_marker( + "tool_call_parsed", + trace_rank, + category="tooling", + task_id=trace_task_id, + tags=["tool_call"], + attrs={"tool_call_count": len(parsed)}, + ) + accumulated_calls.extend(parsed) + yield from _finish_message(response) + break + if tool_parser.unparsed_is_text: + logger.info( + "Unmarked block ended without parsing as a tool call, " + f"emitting it as content (generated_chars={len(combined)})" + ) + yield response.model_copy( + update={ + "text": _block_as_content(combined, tool_parser), + "token": 0, + "finish_reason": None, + } + ) + yield from _finish_message(response) + break logger.info( "tool call parsing interrupted, yield partial tool call as text" ) - response = response.model_copy( + if accumulated_calls: + # An earlier block in this message did produce calls, so they + # are delivered rather than lost to the truncated one. The + # finish reason is withheld here or the consumer stops on this + # chunk and never sees them. + yield response.model_copy( + update={ + "text": _block_as_content(combined, tool_parser), + "token": 0, + "finish_reason": None, + } + ) + yield from _finish_message(response) + break + yield response.model_copy( update={ - "text": "".join(tool_call_text_parts), + "text": combined, "token": 0, "finish_reason": "error", } ) - yield response + + if accumulated_calls and last_response is not None: + # A finite source can end without ever carrying a finish reason, so the + # calls held for coalescing are released here rather than dropped. + yield ToolCallResponse( + tool_calls=accumulated_calls, + usage=last_response.usage, + stats=last_response.stats, + ) diff --git a/src/skulk/worker/runner/llm_inference/runner.py b/src/skulk/worker/runner/llm_inference/runner.py index 3c8149b51..8eced431e 100644 --- a/src/skulk/worker/runner/llm_inference/runner.py +++ b/src/skulk/worker/runner/llm_inference/runner.py @@ -88,7 +88,11 @@ ) from .batch_generator import Cancelled, Finished -from .tool_parsers import make_mlx_parser +from .tool_parsers import ( + UNMARKED_TOOL_DIALECT, + make_mlx_parser, + make_text_dialect_parser, +) def _should_skip_llm_warmup( @@ -796,11 +800,17 @@ def build( and self.tokenizer.tool_call_end and self.tokenizer.tool_parser # type: ignore ): - tool_parser = make_mlx_parser( - self.tokenizer.tool_call_start, - self.tokenizer.tool_call_end, - self.tokenizer.tool_parser, # type: ignore - ) + if self.tokenizer.tool_parser == UNMARKED_TOOL_DIALECT: # type: ignore + tool_parser = make_text_dialect_parser( + self.tokenizer.tool_call_start, + self.tokenizer.tool_call_end, + ) + else: + tool_parser = make_mlx_parser( + self.tokenizer.tool_call_start, + self.tokenizer.tool_call_end, + self.tokenizer.tool_parser, # type: ignore + ) kv_prefix_cache = KVPrefixCache(self.group) diff --git a/src/skulk/worker/runner/llm_inference/tests/test_split_tool_markers.py b/src/skulk/worker/runner/llm_inference/tests/test_split_tool_markers.py new file mode 100644 index 000000000..f8101f5e4 --- /dev/null +++ b/src/skulk/worker/runner/llm_inference/tests/test_split_tool_markers.py @@ -0,0 +1,644 @@ +"""Coverage for tool-call markers that arrive split across chunks. + +A generation chunk is whatever the streaming detokenizer could resolve that +step, not a token. A marker that is a single token id still reaches the parser +as several chunks (``), which was observed live on a +Qwen model served by the MLX engine: the block never opened and the caller +received the raw markup as content. These tests feed markers split the way the +detokenizer actually splits them. +""" + +from __future__ import annotations + +from collections.abc import Generator + +from skulk.api.types import FinishReason, ToolCallItem +from skulk.shared.types.worker.runner_response import ( + GenerationResponse, + ToolCallResponse, +) +from skulk.worker.runner.llm_inference.model_output_parsers import ( + ParserChunk, + parse_tool_calls, +) +from skulk.worker.runner.llm_inference.tool_parsers import ( + ToolParser, + make_mlx_parser, + make_text_dialect_parser, +) + +WEATHER = [ + { + "type": "function", + "function": { + "name": "get_weather", + "parameters": { + "type": "object", + "properties": {"location": {"type": "string"}}, + }, + }, + } +] + + +def chunk( + text: str, + finish_reason: FinishReason | None = None, + *, + thinking: bool = False, +) -> GenerationResponse: + return GenerationResponse( + text=text, + token=1, + finish_reason=finish_reason, + usage=None, + is_thinking=thinking, + ) + + +def feed(pieces: list[str], parser: ToolParser) -> list[ParserChunk]: + def source() -> Generator[ParserChunk]: + for piece in pieces[:-1]: + yield chunk(piece) + yield chunk(pieces[-1], finish_reason="stop") + + return list(parse_tool_calls(source(), parser, tools=WEATHER)) + + +def calls_of(chunks: list[ParserChunk]) -> list[str]: + names: list[str] = [] + for item in chunks: + if isinstance(item, ToolCallResponse): + names.extend(call.name for call in item.tool_calls) + return names + + +def text_of(chunks: list[ParserChunk]) -> str: + return "".join( + getattr(item, "text", "") or "" for item in chunks if item is not None + ) + + +def generic_parser() -> ToolParser: + def inner(text: str) -> dict[str, object]: + from skulk.worker.runner.llm_inference.tool_text_parser import ( + parse_tool_calls_from_text, + ) + + items = parse_tool_calls_from_text(f"{text}") + if not items: + raise ValueError("no tool calls") + item: ToolCallItem = items[0] + return {"name": item.name, "arguments": item.arguments} + + return make_mlx_parser("", "", inner) + + +class TestSplitMarkers: + def test_a_marker_split_across_chunks_still_opens_the_block(self) -> None: + # Exactly the split seen live from the MLX detokenizer. + chunks = feed( + [ + "\n\n\nDenver\n", + "\n\n", + ], + generic_parser(), + ) + assert calls_of(chunks) == ["get_weather"] + assert "" not in text_of(chunks) + + def test_a_closing_marker_split_across_chunks_still_closes(self) -> None: + chunks = feed( + [ + "", + "Denver", + "", + ], + generic_parser(), + ) + assert calls_of(chunks) == ["get_weather"] + + def test_an_unmarked_call_split_across_chunks_is_parsed(self) -> None: + chunks = feed( + ["<|py", "thon_tag|>", '{"name": "get_weather", ', '"parameters": {}}'], + make_text_dialect_parser("{", "<|eom_id|>"), + ) + assert calls_of(chunks) == ["get_weather"] + assert "python_tag" not in text_of(chunks) + + +class TestOrdinaryAnswersStillStream: + def test_prose_is_released_as_soon_as_it_cannot_be_a_marker(self) -> None: + chunks = feed(["The ", "weather ", "is fine."], generic_parser()) + assert calls_of(chunks) == [] + assert text_of(chunks) == "The weather is fine." + + def test_text_that_starts_like_a_marker_and_diverges_is_not_swallowed(self) -> None: + chunks = feed([" None: + chunks = feed([" "] * 40, generic_parser()) + assert calls_of(chunks) == [] + assert text_of(chunks).strip() == "" + + def test_a_message_ending_while_still_ambiguous_is_released(self) -> None: + chunks = feed([" list[ParserChunk]: + def source() -> Generator[ParserChunk]: + yield chunk("Let me think about which tool fits.", thinking=True) + yield chunk(" Probably the weather one.", thinking=True) + for piece in pieces[:-1]: + yield chunk(piece) + yield chunk(pieces[-1], finish_reason="stop") + + return list(parse_tool_calls(source(), generic_parser(), tools=WEATHER)) + + def test_a_call_after_reasoning_is_still_parsed(self) -> None: + chunks = self.run_with_reasoning( + [ + "", + "Denver", + "", + ] + ) + assert calls_of(chunks) == ["get_weather"] + + def test_the_reasoning_still_reaches_the_caller(self) -> None: + chunks = self.run_with_reasoning( + ["", "", ""] + ) + reasoning = "".join( + item.text + for item in chunks + if isinstance(item, GenerationResponse) and item.is_thinking + ) + assert "Let me think" in reasoning + + def test_a_call_only_contemplated_in_reasoning_is_not_executed(self) -> None: + def source() -> Generator[ParserChunk]: + yield chunk("", thinking=True) + yield chunk("I will not call it.", finish_reason="stop") + + chunks = list(parse_tool_calls(source(), generic_parser(), tools=WEATHER)) + assert calls_of(chunks) == [] + + +class TestVisiblePreamble: + """A sentence before the call must not hide it. + + Models routinely announce what they are about to do ("I'll check that.") + and then call. The opening scan therefore has to keep looking after + ordinary text has been released, not decide once and stop. + """ + + def test_a_call_after_a_visible_preamble_is_parsed(self) -> None: + chunks = feed( + [ + "I'll check ", + "that for you. ", + "", + "Denver", + "", + ], + generic_parser(), + ) + assert calls_of(chunks) == ["get_weather"] + + def test_the_preamble_still_reaches_the_caller(self) -> None: + chunks = feed( + [ + "I'll check that. ", + "", + ], + generic_parser(), + ) + assert "I'll check that." in text_of(chunks) + + def test_a_preamble_and_a_split_marker_together(self) -> None: + chunks = feed( + [ + "Sure thing. ", + "", + "", + ], + generic_parser(), + ) + assert calls_of(chunks) == ["get_weather"] + assert "Sure thing." in text_of(chunks) + + def test_two_calls_in_one_message_are_both_parsed(self) -> None: + chunks = feed( + [ + "", + " and also ", + "", + ], + generic_parser(), + ) + assert calls_of(chunks) == ["get_weather", "get_weather"] + + +class TestUnmarkedDialectStaysAnchored: + """The unmarked dialect opens on `{`, so it must only open at the start. + + Letting a brace open a block anywhere would turn any answer that mentions + one into a tool call. + """ + + def test_a_brace_mid_answer_is_not_a_call(self) -> None: + chunks = feed( + ["The set is ", '{"name": "get_weather", "parameters": {}}'], + make_text_dialect_parser("{", "<|eom_id|>"), + ) + assert calls_of(chunks) == [] + assert "The set is" in text_of(chunks) + + def test_a_call_at_the_start_still_opens(self) -> None: + chunks = feed( + ['{"name": "get_weather", ', '"parameters": {"location": "Denver"}}'], + make_text_dialect_parser("{", "<|eom_id|>"), + ) + assert calls_of(chunks) == ["get_weather"] + + def test_the_distinctive_marker_still_opens_after_a_preamble(self) -> None: + # <|python_tag|> is unambiguous, so unlike the brace it may appear + # after a sentence and still open the call. + chunks = feed( + [ + "Let me look that up. ", + '<|python_tag|>{"name": "get_weather", "parameters": {}}', + ], + make_text_dialect_parser("{", "<|eom_id|>"), + ) + assert calls_of(chunks) == ["get_weather"] + + +class TestTextAfterTheCall: + """A model may keep writing after closing the call. + + Requiring the block to end at the closing marker swallowed everything that + followed, so trailing text was lost and a second call in the same message + was folded into the first block. + """ + + def test_trailing_text_after_the_call_is_delivered(self) -> None: + chunks = feed( + [ + "", + " Done.", + ], + generic_parser(), + ) + assert calls_of(chunks) == ["get_weather"] + assert "Done." in text_of(chunks) + + def test_trailing_text_in_the_closing_chunk_is_delivered(self) -> None: + chunks = feed( + [ + "", + " Done.", + ], + generic_parser(), + ) + assert calls_of(chunks) == ["get_weather"] + assert "Done." in text_of(chunks) + + def test_a_second_call_after_trailing_text_is_also_parsed(self) -> None: + chunks = feed( + [ + "", + " and then ", + "", + ], + generic_parser(), + ) + assert calls_of(chunks) == ["get_weather", "get_weather"] + assert "and then" in text_of(chunks) + + +class TestTailAfterADroppedBlock: + """A block that named no offered tool must not swallow what follows. + + Emitting the trailing text along with the dropped block would keep a real + call in that tail from ever being scanned. + """ + + def test_a_real_call_after_a_dropped_block_is_still_found(self) -> None: + chunks = feed( + [ + "hi", + " then ", + "", + ], + generic_parser(), + ) + assert calls_of(chunks) == ["get_weather"] + + def test_the_dropped_block_still_reaches_the_caller_as_content(self) -> None: + chunks = feed( + [ + "hi", + " done.", + ], + generic_parser(), + ) + assert calls_of(chunks) == [] + assert "print" in text_of(chunks) + assert "done." in text_of(chunks) + + +class TestParallelCallsReachTheCaller: + """Several blocks in one message must arrive as one response. + + The consumer of this stream stops at the first chunk carrying a finish + reason, so a response per block would deliver the first call and drop the + rest. Families that write each parallel call in its own block would lose + every call after the first. + """ + + @staticmethod + def tool_responses(chunks: list[ParserChunk]) -> list[ToolCallResponse]: + return [item for item in chunks if isinstance(item, ToolCallResponse)] + + def test_two_blocks_arrive_as_one_response_carrying_both_calls(self) -> None: + chunks = feed( + [ + "Denver", + "Boston", + ], + generic_parser(), + ) + responses = self.tool_responses(chunks) + assert len(responses) == 1 + assert len(responses[0].tool_calls) == 2 + + def test_nothing_terminates_the_stream_before_the_calls(self) -> None: + # A text chunk carrying a finish reason would end the stream at the + # consumer, so the trailing text is released without one and the tool + # response is the terminal chunk. + chunks = feed( + [ + "", + " and then ", + "", + " done.", + ], + generic_parser(), + ) + index = next( + i for i, item in enumerate(chunks) if isinstance(item, ToolCallResponse) + ) + assert all( + getattr(item, "finish_reason", None) is None + for item in chunks[:index] + if item is not None + ) + assert len(self.tool_responses(chunks)[0].tool_calls) == 2 + + def test_a_single_call_is_unchanged(self) -> None: + chunks = feed( + [""], + generic_parser(), + ) + responses = self.tool_responses(chunks) + assert len(responses) == 1 + assert len(responses[0].tool_calls) == 1 + + +class TestDroppedBlockOnTheTerminalChunk: + """A dropped block must not end the stream while more is coming. + + The consumer stops at the first chunk carrying a finish reason, so a + dropped block emitted with one would hide everything after it, including a + real call in the same message. + """ + + def test_a_real_call_after_a_dropped_block_in_the_last_chunk(self) -> None: + chunks = feed( + [ + "" + "", + ], + generic_parser(), + ) + assert calls_of(chunks) == ["get_weather"] + + def test_the_dropped_block_does_not_carry_the_finish_reason(self) -> None: + chunks = feed( + [ + "" + "", + ], + generic_parser(), + ) + index = next( + i for i, item in enumerate(chunks) if isinstance(item, ToolCallResponse) + ) + assert all( + getattr(item, "finish_reason", None) is None + for item in chunks[:index] + if item is not None + ) + + def test_trailing_text_after_a_dropped_block_still_arrives(self) -> None: + chunks = feed( + [" done."], + generic_parser(), + ) + assert calls_of(chunks) == [] + assert "done." in text_of(chunks) + + +class TestEverythingInOneTerminalChunk: + """The whole message can arrive as a single terminal chunk. + + There is no next chunk to drive the streaming scan there, so the rest of + the message has to be parsed in place. Each exit from the close site used + to decide this for itself, and each got it wrong differently. + """ + + def test_two_calls_in_one_terminal_chunk_are_both_delivered(self) -> None: + chunks = feed( + [ + "Denver" + "Boston", + ], + generic_parser(), + ) + responses = [c for c in chunks if isinstance(c, ToolCallResponse)] + assert len(responses) == 1 + assert len(responses[0].tool_calls) == 2 + + def test_a_call_then_text_in_one_terminal_chunk(self) -> None: + chunks = feed( + [" Done."], + generic_parser(), + ) + assert calls_of(chunks) == ["get_weather"] + assert "Done." in text_of(chunks) + + def test_exactly_one_chunk_carries_the_finish_reason(self) -> None: + for pieces in ( + [" Done."], + [" Done."], + ["The weather is fine."], + ): + chunks = feed(pieces, generic_parser()) + terminals = [ + c + for c in chunks + if isinstance(c, ToolCallResponse) + or (c is not None and c.finish_reason is not None) + ] + assert len(terminals) == 1, pieces + assert terminals[0] is chunks[-1], pieces + + +class TestBlockClosedByEndOfGeneration: + """A block the model never closed must not lose the calls before it. + + Several families end a tool-calling message rather than emitting a closing + marker, so this path is normal rather than exceptional, and it has to hold + the same rules as a marker-closed block. + """ + + @staticmethod + def assert_reaches_the_consumer(chunks: list[ParserChunk]) -> None: + """The consumer stops at the first chunk carrying a finish reason. + + Checking the whole list is not enough: calls yielded after something + terminal never reach a caller. + """ + + index = next( + i for i, item in enumerate(chunks) if isinstance(item, ToolCallResponse) + ) + assert all( + getattr(item, "finish_reason", None) is None + for item in chunks[:index] + if item is not None + ) + + def test_an_earlier_call_survives_a_final_unoffered_block(self) -> None: + chunks = feed( + [ + "", + "", + ], + generic_parser(), + ) + assert calls_of(chunks) == ["get_weather"] + self.assert_reaches_the_consumer(chunks) + + def test_an_earlier_call_survives_a_final_truncated_block(self) -> None: + chunks = feed( + [ + "", + " None: + chunks = feed([" list[ParserChunk]: + def source() -> Generator[ParserChunk]: + yield chunk("") + yield chunk("Denver", finish_reason) + + return list(parse_tool_calls(source(), generic_parser(), tools=WEATHER)) + + def test_a_block_cut_off_at_max_tokens_is_not_a_call(self) -> None: + chunks = self.truncated("length") + assert calls_of(chunks) == [] + + def test_the_same_block_ended_normally_is_a_call(self) -> None: + # The families this exists for end the message rather than closing the + # block, so a normal stop must still produce the call. + chunks = self.truncated("stop") + assert calls_of(chunks) == ["get_weather"] + + +class TestRejectedBlockDoesNotLeakMarkers: + """A block handed back as content must not carry its dialect's markers. + + Found by the harness's tool-contract suite: a Llama model called a tool + that a named tool_choice had narrowed away, the call was correctly + rejected, and the caller received `<|python_tag|>` in the answer text. + """ + + def test_an_unmarked_rejected_call_loses_its_marker(self) -> None: + chunks = feed( + [ + '<|python_tag|>{"name": "print", ', + '"parameters": {"value": "hi"}}', + ], + make_text_dialect_parser("{", "<|eom_id|>"), + ) + assert calls_of(chunks) == [] + answer = text_of(chunks) + assert "<|python_tag|>" not in answer + # The call itself is still shown, so the caller can see what happened. + assert "print" in answer + + def test_a_marked_rejected_call_loses_its_markers(self) -> None: + chunks = feed( + [""], + generic_parser(), + ) + assert calls_of(chunks) == [] + answer = text_of(chunks) + assert "" not in answer + assert "" not in answer + assert "print" in answer + + def test_an_accepted_call_is_unaffected(self) -> None: + chunks = feed( + [""], + generic_parser(), + ) + assert calls_of(chunks) == ["get_weather"] diff --git a/src/skulk/worker/runner/llm_inference/tests/test_tool_parser_invariants.py b/src/skulk/worker/runner/llm_inference/tests/test_tool_parser_invariants.py new file mode 100644 index 000000000..cb7ffc82b --- /dev/null +++ b/src/skulk/worker/runner/llm_inference/tests/test_tool_parser_invariants.py @@ -0,0 +1,179 @@ +"""Invariant sweep over the streaming tool-call parser. + +The parser's bugs have all had the same shape: a message arrives split in a way +nobody wrote a case for, and one exit path handles it differently from the +others. Enumerating cases by hand finds them one at a time. This instead states +what must be true of *every* message however it is split, and checks it across +every single split point of each message, so a new exit path that gets one of +these wrong fails here rather than in review. + +The invariants: + +1. Exactly one chunk terminates the stream, and it is the last one. The + consumer stops at the first chunk carrying a finish reason, so anything + after a terminal chunk is invisible to the caller. +2. The calls delivered do not depend on where the message was split. +3. The markup of an accepted call never reaches the caller as content. A + block naming a tool the caller did not offer is a deliberate exception: it + is delivered verbatim so the caller can see what the model did, which is + why those messages opt out of this one. +""" + +from __future__ import annotations + +from collections.abc import Generator +from typing import Any + +from skulk.api.types import FinishReason, ToolCallItem +from skulk.shared.types.worker.runner_response import ( + GenerationResponse, + ToolCallResponse, +) +from skulk.worker.runner.llm_inference.model_output_parsers import ( + ParserChunk, + parse_tool_calls, +) +from skulk.worker.runner.llm_inference.tool_parsers import ( + ToolParser, + make_mlx_parser, + make_text_dialect_parser, +) + +WEATHER: list[dict[str, Any]] = [ + { + "type": "function", + "function": { + "name": "get_weather", + "parameters": { + "type": "object", + "properties": {"location": {"type": "string"}}, + }, + }, + } +] + +CALL = "Denver" + +DROPPED = "" + +# message, expected call names, whether content may carry markup +MARKED_MESSAGES: list[tuple[str, list[str], bool]] = [ + ("The weather is fine.", [], False), + (CALL, ["get_weather"], False), + (f"I'll check. {CALL}", ["get_weather"], False), + (f"{CALL} Done.", ["get_weather"], False), + (f"{CALL}{CALL}", ["get_weather", "get_weather"], False), + (f"{CALL} and then {CALL}", ["get_weather", "get_weather"], False), + (DROPPED, [], True), + (f"{DROPPED}{CALL}", ["get_weather"], True), + ("Braces {like this} are fine.", [], False), +] + +UNMARKED_CALL = '{"name": "get_weather", "parameters": {"location": "Denver"}}' +UNMARKED_MESSAGES: list[tuple[str, list[str], bool]] = [ + ("Just an answer.", [], False), + (UNMARKED_CALL, ["get_weather"], False), + (f"{UNMARKED_CALL} Done.", ["get_weather"], False), + ('{"city": "Denver", "population": 715522}', [], False), + ("The set is {1, 2, 3}.", [], False), + (f"Let me look. <|python_tag|>{UNMARKED_CALL}", ["get_weather"], False), +] + + +def generic_parser() -> ToolParser: + def inner(text: str) -> dict[str, object]: + from skulk.worker.runner.llm_inference.tool_text_parser import ( + parse_tool_calls_from_text, + ) + + items = parse_tool_calls_from_text(f"{text}") + if not items: + raise ValueError("no tool calls") + item: ToolCallItem = items[0] + return {"name": item.name, "arguments": item.arguments} + + return make_mlx_parser("", "", inner) + + +def chunk(text: str, finish_reason: FinishReason | None = None) -> GenerationResponse: + return GenerationResponse( + text=text, token=1, finish_reason=finish_reason, usage=None + ) + + +def run(pieces: list[str], parser: ToolParser) -> list[ParserChunk]: + def source() -> Generator[ParserChunk]: + for piece in pieces[:-1]: + yield chunk(piece) + yield chunk(pieces[-1], finish_reason="stop") + + return list(parse_tool_calls(source(), parser, tools=WEATHER)) + + +def splits(message: str) -> list[list[str]]: + """Every single split point, plus whole and character-by-character.""" + + variants: list[list[str]] = [[message]] + variants.extend( + [message[:index], message[index:]] for index in range(1, len(message)) + ) + variants.append(list(message)) + return variants + + +def call_names(chunks: list[ParserChunk]) -> list[str]: + names: list[str] = [] + for item in chunks: + if isinstance(item, ToolCallResponse): + names.extend(call.name for call in item.tool_calls) + return names + + +def content(chunks: list[ParserChunk]) -> str: + return "".join( + item.text + for item in chunks + if isinstance(item, GenerationResponse) and not item.is_thinking + ) + + +def terminals(chunks: list[ParserChunk]) -> list[ParserChunk]: + return [ + item + for item in chunks + if isinstance(item, ToolCallResponse) + or (item is not None and item.finish_reason is not None) + ] + + +def check_message( + message: str, expected: list[str], parser: ToolParser, markup_ok: bool +) -> None: + for pieces in splits(message): + chunks = run(pieces, parser) + where = f"{message!r} split as {pieces!r}" + + found = terminals(chunks) + assert len(found) == 1, f"{len(found)} terminal chunks for {where}" + assert found[0] is chunks[-1], f"terminal chunk is not last for {where}" + + assert call_names(chunks) == expected, f"calls differ for {where}" + + if expected and not markup_ok: + answer = content(chunks) + assert "" not in answer, f"markup leaked for {where}" + assert "<|python_tag|>" not in answer, f"markup leaked for {where}" + + +class TestMarkedDialectInvariants: + def test_every_split_of_every_message(self) -> None: + parser = generic_parser() + for message, expected, markup_ok in MARKED_MESSAGES: + check_message(message, expected, parser, markup_ok) + + +class TestUnmarkedDialectInvariants: + def test_every_split_of_every_message(self) -> None: + parser = make_text_dialect_parser("{", "<|eom_id|>") + for message, expected, markup_ok in UNMARKED_MESSAGES: + check_message(message, expected, parser, markup_ok) diff --git a/src/skulk/worker/runner/llm_inference/tests/test_tool_text_parser_dialects.py b/src/skulk/worker/runner/llm_inference/tests/test_tool_text_parser_dialects.py new file mode 100644 index 000000000..f136df6fb --- /dev/null +++ b/src/skulk/worker/runner/llm_inference/tests/test_tool_text_parser_dialects.py @@ -0,0 +1,246 @@ +"""Tool-call dialect coverage for the shared text parser. + +Models do not agree on how to say "call this function". A parser that knows +only one dialect does not merely miss the call: the markup falls through to +`content`, so the caller receives template scaffolding as if it were the +model's answer. This module pins one example per dialect we claim, plus the +guards that stop prose from being read as a call. +""" + +import json + +from skulk.worker.runner.llm_inference.tool_text_parser import ( + parse_tool_calls_from_text, +) + + +def _one(text: str) -> tuple[str, dict[str, object]]: + """Parse text expected to carry exactly one call; return name and args.""" + calls = parse_tool_calls_from_text(text) + assert calls is not None, f"no tool call parsed from {text!r}" + assert len(calls) == 1, f"expected one call, got {len(calls)}" + return calls[0].name, json.loads(calls[0].arguments) + + +class TestLlama: + """Llama 3.1+ marks calls with <|python_tag|> and uses `parameters`.""" + + def test_python_tag_call(self) -> None: + text = ( + '<|python_tag|>{"name": "get_weather", ' + '"parameters": {"location": "Cedar Rapids, Iowa"}}<|eom_id|>' + ) + name, args = _one(text) + assert name == "get_weather" + assert args == {"location": "Cedar Rapids, Iowa"} + + def test_terminator_is_not_swallowed_into_arguments(self) -> None: + # The observed failure leaked <|eom_id|> and the next header into + # content; the parser must stop at the message boundary. + text = ( + '<|python_tag|>{"name": "f", "parameters": {"a": 1}}<|eom_id|>' + "<|start_header_id|>assistant<|end_header_id|>" + ) + _, args = _one(text) + assert args == {"a": 1} + + def test_chained_calls_separated_by_semicolons(self) -> None: + text = ( + '<|python_tag|>{"name": "a", "parameters": {}};' + '{"name": "b", "parameters": {"x": 2}}<|eom_id|>' + ) + calls = parse_tool_calls_from_text(text) + assert calls is not None + assert [call.name for call in calls] == ["a", "b"] + + def test_unmarked_call_object_is_accepted(self) -> None: + name, args = _one('{"name": "get_weather", "parameters": {"location": "X"}}') + assert name == "get_weather" + assert args == {"location": "X"} + + +class TestMistral: + def test_tool_calls_array(self) -> None: + text = '[TOOL_CALLS] [{"name": "get_weather", "arguments": {"location": "Paris"}}]' + name, args = _one(text) + assert name == "get_weather" + assert args == {"location": "Paris"} + + def test_multiple_calls_in_one_array(self) -> None: + text = '[TOOL_CALLS] [{"name": "a", "arguments": {}}, {"name": "b", "arguments": {}}]' + calls = parse_tool_calls_from_text(text) + assert calls is not None + assert [call.name for call in calls] == ["a", "b"] + + def test_trailing_prose_after_the_array_is_ignored(self) -> None: + text = '[TOOL_CALLS] [{"name": "a", "arguments": {}}]\nI will check that.' + name, _ = _one(text) + assert name == "a" + + +class TestGlm: + def test_arg_key_value_pairs(self) -> None: + text = ( + "get_weather\n" + "locationCedar Rapids\n" + "unitcelsius\n" + "" + ) + name, args = _one(text) + assert name == "get_weather" + assert args == {"location": "Cedar Rapids", "unit": "celsius"} + + +class TestExistingDialectsStillWork: + """The new branches must not shadow the dialects that already worked.""" + + def test_hermes_json_block(self) -> None: + name, args = _one('{"name": "f", "arguments": {"a": 1}}') + assert name == "f" + assert args == {"a": 1} + + def test_qwen3_xml_block(self) -> None: + text = ( + "" + "Cedar Rapids" + "" + ) + name, args = _one(text) + assert name == "get_weather" + assert args == {"location": "Cedar Rapids"} + + +class TestFalsePositiveGuards: + """Prose must never be read as a tool call.""" + + def test_plain_prose_is_not_a_call(self) -> None: + assert parse_tool_calls_from_text("The weather in Cedar Rapids is fine.") is None + + def test_json_answer_embedded_in_prose_is_not_a_call(self) -> None: + text = 'Here is the record you asked for: {"name": "Ada", "parameters": {}}' + assert parse_tool_calls_from_text(text) is None + + def test_json_object_without_a_name_is_not_a_call(self) -> None: + assert parse_tool_calls_from_text('{"location": "Paris"}') is None + + def test_json_answer_with_a_name_but_no_arguments_is_not_a_call(self) -> None: + # A model answering with a record that happens to have a name field. + assert parse_tool_calls_from_text('{"name": "Ada", "born": 1815}') is None + + def test_empty_text(self) -> None: + assert parse_tool_calls_from_text("") is None + + +class TestOfferedToolsOnly: + """A parsed call must name a tool the caller actually offered.""" + + WEATHER = [ + { + "type": "function", + "function": { + "name": "get_weather", + "parameters": { + "type": "object", + "properties": {"location": {"type": "string"}}, + }, + }, + } + ] + + def test_a_llama_builtin_is_not_a_tool_call(self) -> None: + # Llama answers some plain questions with its own `print` built-in. + # Reporting that as a call hands the caller a name they cannot run. + assert ( + parse_tool_calls_from_text( + '<|python_tag|>{"name": "print", "parameters": {"value": "hi"}}', + self.WEATHER, + ) + is None + ) + + def test_an_offered_tool_still_parses(self) -> None: + calls = parse_tool_calls_from_text( + '<|python_tag|>{"name": "get_weather", "parameters": {"location": "x"}}', + self.WEATHER, + ) + assert calls is not None + assert [call.name for call in calls] == ["get_weather"] + + def test_the_offered_call_survives_alongside_a_builtin(self) -> None: + calls = parse_tool_calls_from_text( + '<|python_tag|>{"name": "print", "parameters": {}};' + '{"name": "get_weather", "parameters": {"location": "x"}}', + self.WEATHER, + ) + assert calls is not None + assert [call.name for call in calls] == ["get_weather"] + + def test_an_absent_tools_list_is_not_a_statement_that_nothing_may_be_called( + self, + ) -> None: + # This is a shared helper: the steward parses its own turns through the + # same dialects without passing a tools list. Whether a request that + # declared no tools may return a call is decided by the caller, which + # is the only place that knows. + calls = parse_tool_calls_from_text( + '<|python_tag|>{"name": "print", "parameters": {"value": "hi"}}' + ) + assert calls is not None + assert [call.name for call in calls] == ["print"] + + + +class TestUnmarkedCallFollowedByText: + """A model may keep writing after an unmarked call. + + Observed live: a Llama model asked to call a tool and then say a word wrote + `{"name": ...}Done`. Requiring the object to be the entire message dropped + a perfectly good call and returned it as content. + """ + + WEATHER = [ + { + "type": "function", + "function": { + "name": "get_weather", + "parameters": { + "type": "object", + "properties": {"location": {"type": "string"}}, + }, + }, + } + ] + + def test_a_leading_call_object_with_trailing_text_is_a_call(self) -> None: + calls = parse_tool_calls_from_text( + '{"name": "get_weather", "parameters": {"location": "Denver"}}Done', + self.WEATHER, + ) + assert calls is not None + assert [call.name for call in calls] == ["get_weather"] + + def test_prose_before_the_object_is_still_not_a_call(self) -> None: + assert ( + parse_tool_calls_from_text( + 'Here is some JSON: {"name": "get_weather", "parameters": {}}', + self.WEATHER, + ) + is None + ) + + def test_a_json_answer_is_still_not_a_call(self) -> None: + assert ( + parse_tool_calls_from_text( + '{"city": "Denver", "population": 715522}', self.WEATHER + ) + is None + ) + + def test_an_object_naming_no_offered_tool_is_still_not_a_call(self) -> None: + assert ( + parse_tool_calls_from_text( + '{"name": "report", "parameters": {"n": 1}} and more text', + self.WEATHER, + ) + is None + ) diff --git a/src/skulk/worker/runner/llm_inference/tests/test_unmarked_tool_dialect.py b/src/skulk/worker/runner/llm_inference/tests/test_unmarked_tool_dialect.py new file mode 100644 index 000000000..4207ac8c6 --- /dev/null +++ b/src/skulk/worker/runner/llm_inference/tests/test_unmarked_tool_dialect.py @@ -0,0 +1,130 @@ +"""Coverage for the unmarked tool-call dialect. + +Llama 3.1+ writes a tool call as a bare JSON object with no opening marker and +ends the message with ``<|eom_id|>`` rather than a closing marker, so neither +half of the marker mechanism applies. These tests pin the two behaviors that +makes possible: a call that opens on ``{`` and is closed by the end of +generation is parsed, and a block that turns out not to be a call is delivered +as content instead of being reported as a failure. +""" + +from __future__ import annotations + +from collections.abc import Generator + +from skulk.api.types import FinishReason, ToolCallItem +from skulk.shared.types.worker.runner_response import ( + GenerationResponse, + ToolCallResponse, +) +from skulk.worker.runner.llm_inference.model_output_parsers import ( + ParserChunk, + parse_tool_calls, +) +from skulk.worker.runner.llm_inference.tool_parsers import make_text_dialect_parser + +WEATHER_TOOL = { + "type": "function", + "function": { + "name": "get_weather", + "description": "Get current weather for a location", + "parameters": { + "type": "object", + "properties": { + "location": {"type": "string"}, + "days": {"type": "integer"}, + }, + "required": ["location"], + }, + }, +} + + +def chunk(text: str, finish_reason: FinishReason | None = None) -> GenerationResponse: + return GenerationResponse( + text=text, token=1, finish_reason=finish_reason, usage=None + ) + + +def run(texts: list[str], final: str) -> list[ParserChunk]: + """Feed the chunks through the parser and collect what a caller would see.""" + + def source() -> Generator[ParserChunk]: + for text in texts: + yield chunk(text) + yield chunk(final, finish_reason="stop") + + return list( + parse_tool_calls( + source(), + make_text_dialect_parser("{", "<|eom_id|>"), + tools=[WEATHER_TOOL], + ) + ) + + +def tool_calls(chunks: list[ParserChunk]) -> list[ToolCallItem]: + calls: list[ToolCallItem] = [] + for item in chunks: + if isinstance(item, ToolCallResponse): + calls.extend(item.tool_calls) + return calls + + +def text_of(chunks: list[ParserChunk]) -> str: + return "".join( + getattr(item, "text", "") or "" for item in chunks if item is not None + ) + + +class TestUnmarkedCall: + def test_bare_call_closed_by_end_of_generation_is_parsed(self) -> None: + chunks = run( + ['{"name": "get_weather", ', '"parameters": {"location": '], + '"Cedar Rapids, Iowa"}}', + ) + calls = tool_calls(chunks) + assert [call.name for call in calls] == ["get_weather"] + assert "Cedar Rapids" in calls[0].arguments + + def test_the_call_does_not_also_leak_out_as_content(self) -> None: + # The failure this guards is the one seen live: the call arriving as + # text with finish_reason "stop", so a client sees JSON in the answer + # and no tool call at all. + chunks = run(['{"name": "get_weather", '], '"parameters": {}}') + assert tool_calls(chunks) + assert "get_weather" not in text_of(chunks) + + def test_a_python_tag_call_in_the_same_block_is_parsed(self) -> None: + chunks = run( + ['{"name": "get_weather", "parameters": {}}'], + '<|python_tag|>{"name": "get_weather", "parameters": {"location": "x"}}', + ) + assert [call.name for call in tool_calls(chunks)] == ["get_weather"] + + def test_arguments_are_coerced_to_the_tool_schema(self) -> None: + # Llama writes its arguments under "parameters", and models routinely + # quote numbers. Both have to be normalized before a caller sees the + # call, so this pins that the block goes through schema coercion. + chunks = run(['{"name": "get_weather", '], '"parameters": {"days": "3"}}') + assert '"days": 3' in tool_calls(chunks)[0].arguments + + +class TestNotACall: + def test_a_json_answer_is_delivered_as_content_not_an_error(self) -> None: + # A caller may supply tools and still ask for a JSON answer. Opening the + # block on "{" means that answer lands here, and reporting it as a + # parse failure would turn a correct response into an error. + chunks = run(['{"city": "Cedar Rapids", '], '"population": 137710}') + assert tool_calls(chunks) == [] + assert '"population": 137710' in text_of(chunks) + assert all( + getattr(item, "finish_reason", None) != "error" + for item in chunks + if item is not None + ) + + def test_prose_never_enters_the_block_at_all(self) -> None: + chunks = run(["The weather in "], "Cedar Rapids is fine.") + assert tool_calls(chunks) == [] + assert text_of(chunks) == "The weather in Cedar Rapids is fine." diff --git a/src/skulk/worker/runner/llm_inference/tool_parsers.py b/src/skulk/worker/runner/llm_inference/tool_parsers.py index e937f646c..8a8f5baa7 100644 --- a/src/skulk/worker/runner/llm_inference/tool_parsers.py +++ b/src/skulk/worker/runner/llm_inference/tool_parsers.py @@ -1,16 +1,57 @@ import json import math from dataclasses import dataclass -from typing import Any, Callable +from typing import Any, Callable, cast from skulk.api.types import ToolCallItem +UNMARKED_TOOL_DIALECT = "skulk:unmarked-tool-dialect" +"""Sentinel tool parser meaning "read the whole block with the text dialects". + +A tokenizer carries a callable in its tool-parser slot for marker-delimited +families, and the runner strips the markers before calling it. Llama has no +opening marker to strip, so the runner must build a different parser rather +than call anything; this sentinel is how it tells the two cases apart. +""" + @dataclass class ToolParser: start_parsing: str end_parsing: str _inner_parser: Callable[[str], list[ToolCallItem] | None] + extra_start_parsing: tuple[str, ...] = () + """Further markers that also open a tool-call block. + + A family can open a call more than one way. Llama writes the bare call + object most of the time but prefixes ``<|python_tag|>`` when it reaches for + a tool by name, and a marker that does not open the block is emitted to the + caller as content. + """ + anchored: bool = False + """Whether the primary marker opens a block only at the start of a message. + + A distinctive marker opens a block wherever it appears, because models + routinely write a sentence before calling. The unmarked dialect's marker is + ``{``, which also appears in prose and in JSON answers, so letting it open + a block anywhere would turn any brace mid-answer into a call. The families + using it write the call as the whole message, so anchoring loses nothing. + """ + unparsed_is_text: bool = False + """Whether a block that fails to parse is content rather than a failure. + + Marker-delimited dialects open on a token no ordinary answer emits, so a + block that will not parse is genuinely broken output. Unmarked dialects open + on ``{``, which a model asked for JSON also emits, so there the safe reading + of an unparsable block is that the model simply answered in JSON and the + text should be delivered as content. + """ + + @property + def start_markers(self) -> tuple[str, ...]: + """Every marker whose appearance opens a tool-call block.""" + + return (self.start_parsing, *self.extra_start_parsing) def parse( self, text: str, tools: list[dict[str, Any]] | None @@ -248,8 +289,69 @@ def make_json_parser() -> ToolParser: ) +def make_text_dialect_parser(tool_call_start: str, tool_call_end: str) -> ToolParser: + """Build a parser that reads the whole block with the cross-family dialects. + + Unlike :func:`make_mlx_parser`, the markers are not stripped before parsing: + for several families the opening marker is part of the call itself (Llama + writes the bare call object, so its opening marker is ``{``), and the + dialect detection in :func:`parse_tool_calls_from_text` keys off the markers + that are present. A block that does not parse is treated as content. + """ + + # Imported at call time: tool_text_parser imports the schema coercion from + # this module, so a module-level import here would be circular. + from skulk.worker.runner.llm_inference.tool_text_parser import ( + parse_tool_calls_from_text, + ) + + return ToolParser( + start_parsing=tool_call_start, + end_parsing=tool_call_end, + _inner_parser=lambda text: parse_tool_calls_from_text(text), + extra_start_parsing=("<|python_tag|>",), + anchored=True, + unparsed_is_text=True, + ) + + def infer_tool_parser(chat_template: str) -> ToolParser | None: """Attempt to auto-infer a tool parser from the chat template.""" if "" in chat_template and "tool_call.name" in chat_template: return make_json_parser() return None + + +def declared_tool_calls( + tool_calls: list[ToolCallItem], tools: list[dict[str, Any]] | None +) -> list[ToolCallItem]: + """Keep only calls naming a tool the caller actually offered. + + Some families reach for a built-in the caller never declared: Llama answers + a plain question with ``<|python_tag|>print("hello")``, which parses as a + call to ``print``. Surfacing that as a tool call hands the caller a name + they have no implementation for, so it is dropped here and the block is + delivered as content instead. + + ``tools`` of ``None`` means the caller had no list to check against, not + that nothing may be called: this is a shared helper, and the steward parses + its own turns through the same dialects without passing one. Whether a + request that declared no tools may return a call is decided by the caller, + which is the only place that knows. + """ + + declared: set[str] = set() + if tools is None: + return tool_calls + for tool in tools: + function = tool.get("function") + if not isinstance(function, dict): + continue + name = cast("object", function.get("name")) # pyright: ignore[reportUnknownMemberType] + if isinstance(name, str): + declared.add(name) + if not declared: + # Tools were offered but none is usably named, which is the caller's + # malformed input rather than a statement that nothing may be called. + return tool_calls + return [call for call in tool_calls if call.name in declared] diff --git a/src/skulk/worker/runner/llm_inference/tool_text_parser.py b/src/skulk/worker/runner/llm_inference/tool_text_parser.py index d6939d630..094e4b5d2 100644 --- a/src/skulk/worker/runner/llm_inference/tool_text_parser.py +++ b/src/skulk/worker/runner/llm_inference/tool_text_parser.py @@ -26,10 +26,13 @@ import json import re -from typing import Any +from typing import Any, cast from skulk.api.types import ToolCallItem -from skulk.worker.runner.llm_inference.tool_parsers import coerce_tool_calls_to_schema +from skulk.worker.runner.llm_inference.tool_parsers import ( + coerce_tool_calls_to_schema, + declared_tool_calls, +) # gpt-oss harmony tool call: the recipient `to=functions.NAME` and a `commentary` # channel together, then a `<|message|>` body holding the JSON arguments (up to @@ -63,6 +66,23 @@ _PARAMETER_RE = re.compile( r"\s]+)\s*>\s*(.*?)\s*", re.DOTALL ) +# Llama 3.1+ marks a tool call with <|python_tag|> and ends the turn with +# <|eom_id|> (end of MESSAGE, handing off to a tool) rather than <|eot_id|> +# (end of TURN, handing back to the user). The body is one or more JSON +# objects using "parameters" rather than "arguments"; several calls are +# separated by ";". +_PYTHON_TAG_RE = re.compile( + r"<\|python_tag\|>(.*?)(?=<\|eom_id\|>|<\|eot_id\|>|<\|start_header_id\|>|$)", + re.DOTALL, +) +# Mistral emits a JSON array behind a [TOOL_CALLS] marker. +_MISTRAL_RE = re.compile(r"\[TOOL_CALLS\]\s*(\[.*)", re.DOTALL) +# GLM puts the function name on its own line inside , then names +# arguments in / pairs rather than as JSON. +_GLM_ARG_RE = re.compile( + r"\s*(.*?)\s*\s*\s*(.*?)\s*", + re.DOTALL, +) def _first_json_object(text: str) -> dict[str, Any] | None: @@ -107,6 +127,104 @@ def _first_json_object(text: str) -> dict[str, Any] | None: return None +def _call_from_json_object(obj: object) -> ToolCallItem | None: + """Build a call from the ``{"name": ..., "arguments"/"parameters": ...}`` shape. + + Shared by every JSON-carrying dialect (Hermes, Llama, Mistral), which + differ only in the markup around this object. Llama uses ``parameters`` + where Hermes uses ``arguments``; both are accepted. + + ``ToolCallItem.arguments`` must decode to a JSON object downstream (schema + coercion, the Claude adapter's dict input). A dict is re-serialized; the + OpenAI shape where ``arguments`` is already a JSON-encoded string is kept + as-is when it decodes to an object; any other shape (list, scalar, or a + string that is not a JSON object) is malformed and falls back to ``{}`` + rather than being invented. + """ + + if not isinstance(obj, dict): + return None + payload = cast("dict[str, Any]", obj) + if not isinstance(payload.get("name"), str): + return None + args = payload.get("arguments", payload.get("parameters", {})) + if isinstance(args, dict): + args_str = json.dumps(args) + elif isinstance(args, str) and _first_json_object(args) is not None: + args_str = args + else: + args_str = "{}" + return ToolCallItem(name=str(payload["name"]), arguments=args_str) + + +def _python_tag_calls(text: str) -> list[ToolCallItem]: + """Parse Llama 3.1+ ``<|python_tag|>`` calls.""" + + calls: list[ToolCallItem] = [] + for match in _PYTHON_TAG_RE.finditer(text): + for chunk in match.group(1).split(";"): + call = _call_from_json_object(_first_json_object(chunk)) + if call is not None: + calls.append(call) + return calls + + +def _mistral_calls(text: str) -> list[ToolCallItem]: + """Parse Mistral ``[TOOL_CALLS] [...]`` arrays.""" + + match = _MISTRAL_RE.search(text) + if match is None: + return [] + decoder = json.JSONDecoder() + try: + array, _ = decoder.raw_decode(match.group(1).strip()) + except ValueError: + return [] + if not isinstance(array, list): + return [] + calls: list[ToolCallItem] = [] + for entry in array: + call = _call_from_json_object(entry) + if call is not None: + calls.append(call) + return calls + + +def _bare_json_call(text: str) -> list[ToolCallItem]: + """Parse an unmarked call that opens the message. + + Llama omits ``<|python_tag|>`` in some templates and simply emits the call + object. The message must *begin* with that object, so prose containing JSON + is never read as a call, but the model may keep writing after it: a call + followed by a closing remark is still a call, and requiring the object to + be the entire message lost it. + + Two things keep this from mistaking a JSON answer for a call. The object + must carry a ``name`` alongside an ``arguments`` or ``parameters`` value, + which an answer rarely has; and the caller's tools are checked afterwards, + so an object naming nothing the caller offered is dropped and delivered as + content. + """ + + stripped = text.strip() + if not stripped.startswith("{"): + return [] + decoder = json.JSONDecoder() + try: + obj, _ = decoder.raw_decode(stripped) + except ValueError: + return [] + if not isinstance(obj, dict): + return [] + payload = cast("dict[str, Any]", obj) + if "name" not in payload: + return [] + if not isinstance(payload.get("arguments", payload.get("parameters")), (dict, str)): + return [] + call = _call_from_json_object(payload) + return [call] if call is not None else [] + + def _harmony_tool_calls(text: str) -> list[ToolCallItem]: calls: list[ToolCallItem] = [] seen: set[tuple[int, str]] = set() @@ -152,22 +270,23 @@ def _toolcall_block_calls(text: str) -> list[ToolCallItem]: calls.append(ToolCallItem(name=name, arguments=json.dumps(params))) continue # Hermes / older Qwen JSON form: {"name": ..., "arguments": {...}}. - obj = _first_json_object(inner) - if isinstance(obj, dict) and isinstance(obj.get("name"), str): - args = obj.get("arguments", obj.get("parameters", {})) - # ToolCallItem.arguments must decode to a JSON object downstream - # (schema coercion, the Claude adapter's dict input). A dict is - # re-serialized; the OpenAI shape where `arguments` is already a - # JSON-encoded string (e.g. "{\"city\":\"Paris\"}") is kept as-is - # when it decodes to an object; any other shape (list/scalar, or a - # string that is not a JSON object) is malformed and falls back to {}. - if isinstance(args, dict): - args_str = json.dumps(args) - elif isinstance(args, str) and _first_json_object(args) is not None: - args_str = args - else: - args_str = "{}" - calls.append(ToolCallItem(name=obj["name"], arguments=args_str)) + # GLM names arguments in / pairs with the function + # name on the first line, so there is no JSON object to find. Checked + # before the JSON scan because a value may itself contain JSON. + arg_pairs = _GLM_ARG_RE.findall(inner) + if arg_pairs: + name = inner.split("", 1)[0].strip().splitlines() + if name and name[-1].strip(): + params = {key: value for key, value in arg_pairs} + calls.append( + ToolCallItem( + name=name[-1].strip(), arguments=json.dumps(params) + ) + ) + continue + call = _call_from_json_object(_first_json_object(inner)) + if call is not None: + calls.append(call) return calls @@ -176,11 +295,24 @@ def parse_tool_calls_from_text( ) -> list[ToolCallItem] | None: """Recover tool calls a reasoning model emitted as text (llama.cpp engine). - Detects the format from the markers present (a harmony ``to=functions.`` - channel, or a ```` block in JSON or Qwen3 XML), parses the calls, - and coerces argument types to the tool schema. Returns ``None`` when no tool - call is present (the model answered in prose), so the caller can fall back to - emitting the content. + Detects the dialect from the markers present and parses the calls, then + coerces argument types to the tool schema. Recognized dialects: + + - harmony ``to=functions.`` channels (gpt-oss) + - ```` blocks carrying Hermes JSON, Qwen3 XML, or GLM + ````/```` pairs + - Llama ``<|python_tag|>`` calls, which use ``parameters`` rather than + ``arguments`` and may chain several with ``;`` + - Mistral ``[TOOL_CALLS]`` arrays + - an unmarked call object opening the message, which the model may keep + writing after + + When ``tools`` is given, calls naming a tool the caller did not offer are + dropped, because a model reaching for one of its own built-ins has not + called anything the caller can run. + + Returns ``None`` when no tool call is present (the model answered in prose), + so the caller can fall back to emitting the content. """ if not text: return None @@ -189,8 +321,26 @@ def parse_tool_calls_from_text( calls = _harmony_tool_calls(text) if not calls and "" in text: calls = _toolcall_block_calls(text) + if not calls and "<|python_tag|>" in text: + calls = _python_tag_calls(text) + if not calls and "[TOOL_CALLS]" in text: + calls = _mistral_calls(text) + if not calls: + # Last resort, and deliberately narrow: the message must begin with the + # call object. Unmarked dialects are otherwise indistinguishable from a + # model answering in JSON, so anything looser invents tool calls from + # prose. The object must also carry a name alongside arguments, and the + # caller's tools are checked afterwards. + calls = _bare_json_call(text) if not calls: return None if tools is not None: + # A model may reach for one of its own built-ins: Llama answers some + # plain questions with a call to `print`, and gpt-oss has `python` and + # `browser`. Those name nothing the caller can run, so a block left with + # no offered tool reads as prose and the caller gets the text instead. + calls = declared_tool_calls(calls, tools) + if not calls: + return None calls = coerce_tool_calls_to_schema(calls, tools) return calls diff --git a/src/skulk/worker/tests/unittests/test_runner/test_finish_reason_sse.py b/src/skulk/worker/tests/unittests/test_runner/test_finish_reason_sse.py index 1703e4c66..a28ecb18f 100644 --- a/src/skulk/worker/tests/unittests/test_runner/test_finish_reason_sse.py +++ b/src/skulk/worker/tests/unittests/test_runner/test_finish_reason_sse.py @@ -3,6 +3,12 @@ from mlx_lm.tokenizer_utils import TokenizerWrapper +from skulk.api.types import ( + CompletionTokensDetails, + PromptTokensDetails, + ToolCallItem, + Usage, +) from skulk.shared.models.model_cards import ( ModelCard, ModelTask, @@ -30,6 +36,7 @@ parse_gemma4_thinking_channels, parse_thinking_models, parse_tool_calls, + reject_unoffered_tool_calls, ) from skulk.worker.runner.llm_inference.tool_parsers import make_mlx_parser @@ -447,7 +454,10 @@ def test_apply_all_parsers_uses_deepseek_parser_from_family_without_model_class( tokenizer=_no_thinking_tokenizer(), model_type=Model, model_id=ModelId("custom/deepseek-compatible"), - tools=None, + # The tool has to be offered: a request declaring none cannot + # produce a call on any path. What this covers is that the + # DeepSeek parser is selected from the family alone. + tools=[{"type": "function", "function": {"name": "get_weather"}}], model_card=ModelCard( model_id=ModelId("custom/deepseek-compatible"), storage_size=Memory.from_bytes(1024), @@ -602,3 +612,198 @@ def test_finish_reason_with_buffered_tokens_drain_loop(self): assert _got_finish(collected), ( f"No finish_reason in collected: {[(type(r).__name__, getattr(r, 'finish_reason', None) if isinstance(r, GenerationResponse) else 'tool') for r in collected]}" ) + + +class TestToolParsingRequiresOfferedTools: + """A request that declared no tools must not come back with a tool call. + + The tool parser is wired from the tokenizer, which does not know what this + request asked for, so without gating on the request a model that + spontaneously writes something call-shaped (exactly what a request asking + for JSON output invites) returns `tool_calls` to a caller who offered none. + It is also what makes `tool_choice: "none"` hold, since resolving that + choice removes the tools from the request. + """ + + @staticmethod + def _run( + tools: list[dict[str, Any]] | None, + ) -> list[GenerationResponse | ToolCallResponse]: + tokens = [ + _make_response("", 200), + _make_response("anything", 201), + _make_response("", 202, finish_reason="stop"), + ] + return _step_until_finish( + apply_all_parsers( + _queue_source(tokens), + prompt="", + tool_parser=_dummy_parser, + tokenizer=_no_thinking_tokenizer(), + model_type=Model, + model_id=ModelId("mlx-community/does-not-matter"), + tools=tools, + ) + ) + + def test_no_tools_offered_yields_no_tool_call(self) -> None: + results = self._run(None) + assert not any(isinstance(item, ToolCallResponse) for item in results) + + def test_an_empty_tools_list_yields_no_tool_call(self) -> None: + results = self._run([]) + assert not any(isinstance(item, ToolCallResponse) for item in results) + + def test_no_tools_offered_still_strips_the_markers(self) -> None: + # Skipping the scan entirely left the dialect's markers in the answer, + # which a caller saw. The block is recognized either way; only whether + # it may become a call depends on the request. + results = self._run(None) + text = "".join( + item.text for item in results if isinstance(item, GenerationResponse) + ) + assert "" not in text + assert "" not in text + + def test_offering_a_tool_still_yields_the_call(self) -> None: + results = self._run( + [{"type": "function", "function": {"name": "test_fn"}}] + ) + assert any(isinstance(item, ToolCallResponse) for item in results) + + +class TestFamilyParsersHonourOfferedTools: + """gpt-oss and DeepSeek parse their own calls, so they need the same rule. + + Observed live on gpt-oss served by MLX: a request sending + `tool_choice: "none"`, which removes the tools, still came back with a + call, and its name carried the harmony namespace prefix as well. Those + parsers are selected before the marker path, so the offered-tools filter + the marker path applies never saw them. The guard is tested directly + rather than through a synthetic token stream, because these parsers decode + real harmony/DSML tokens and a hand-built stream would pass vacuously. + """ + + @staticmethod + def _run( + calls: list[str], tools: list[dict[str, Any]] | None + ) -> list[GenerationResponse | ToolCallResponse]: + def source() -> Generator[GenerationResponse | ToolCallResponse | None]: + yield _make_response("thinking about it", 0) + yield ToolCallResponse( + tool_calls=[ + ToolCallItem(name=name, arguments="{}") for name in calls + ], + usage=None, + stats=None, + ) + + return [ + item + for item in reject_unoffered_tool_calls(source(), tools) + if item is not None + ] + + def test_no_tools_offered_yields_no_call(self) -> None: + results = self._run(["get_weather"], None) + assert not any(isinstance(item, ToolCallResponse) for item in results) + + def test_a_call_to_an_unoffered_tool_is_dropped(self) -> None: + results = self._run( + ["get_weather"], + [{"type": "function", "function": {"name": "something_else"}}], + ) + assert not any(isinstance(item, ToolCallResponse) for item in results) + + def test_an_offered_tool_still_produces_the_call(self) -> None: + results = self._run( + ["get_weather"], + [{"type": "function", "function": {"name": "get_weather"}}], + ) + calls = [item for item in results if isinstance(item, ToolCallResponse)] + assert [c.name for c in calls[0].tool_calls] == ["get_weather"] + + def test_only_the_unoffered_call_is_dropped(self) -> None: + results = self._run( + ["something_else", "get_weather"], + [{"type": "function", "function": {"name": "get_weather"}}], + ) + calls = [item for item in results if isinstance(item, ToolCallResponse)] + assert [c.name for c in calls[0].tool_calls] == ["get_weather"] + + def test_a_dropped_call_is_delivered_as_content(self) -> None: + # Dropping the only output would answer the request with a blank + # message, so the caller is shown what the model actually did. + results = self._run(["get_weather"], None) + text = "".join( + item.text for item in results if isinstance(item, GenerationResponse) + ) + assert "get_weather" in text + + def test_the_stream_still_terminates_when_a_call_is_dropped(self) -> None: + assert _got_finish(self._run(["get_weather"], None)) + + def test_a_terminal_chunk_after_the_call_is_not_duplicated(self) -> None: + # These streams usually carry a terminal chunk after the call. Adding a + # second terminal would end the stream at the consumer before the real + # one arrives. + def source() -> Generator[GenerationResponse | ToolCallResponse | None]: + yield ToolCallResponse( + tool_calls=[ToolCallItem(name="get_weather", arguments="{}")], + usage=None, + stats=None, + ) + yield _make_response("", 1, finish_reason="stop") + + results = [ + item + for item in reject_unoffered_tool_calls(source(), None) + if item is not None + ] + terminals = [ + item + for item in results + if isinstance(item, ToolCallResponse) or item.finish_reason is not None + ] + assert len(terminals) == 1 + assert terminals[0] is results[-1] + text = "".join( + item.text for item in results if isinstance(item, GenerationResponse) + ) + assert "get_weather" in text + + def test_several_rejected_calls_stay_readable_and_keep_accounting(self) -> None: + # Concatenating the rendered calls without a separator produced text a + # caller could not read back, and the fabricated fallback threw away + # the accounting the rejected response carried. + usage = Usage( + prompt_tokens=7, + completion_tokens=3, + total_tokens=10, + prompt_tokens_details=PromptTokensDetails(cached_tokens=0), + completion_tokens_details=CompletionTokensDetails(reasoning_tokens=0), + ) + + def source() -> Generator[GenerationResponse | ToolCallResponse | None]: + yield ToolCallResponse( + tool_calls=[ToolCallItem(name="first", arguments="{}")], + usage=None, + stats=None, + ) + yield ToolCallResponse( + tool_calls=[ToolCallItem(name="second", arguments="{}")], + usage=usage, + stats=None, + ) + + results = [ + item + for item in reject_unoffered_tool_calls(source(), None) + if item is not None + ] + assert len(results) == 1 + final = results[0] + assert isinstance(final, GenerationResponse) + assert final.text.count("\n") == 1 + assert "first" in final.text and "second" in final.text + assert final.usage == usage diff --git a/src/skulk/worker/tests/unittests/test_runner/test_parse_tool_calls.py b/src/skulk/worker/tests/unittests/test_runner/test_parse_tool_calls.py index 0a6911a9a..e15d0487a 100644 --- a/src/skulk/worker/tests/unittests/test_runner/test_parse_tool_calls.py +++ b/src/skulk/worker/tests/unittests/test_runner/test_parse_tool_calls.py @@ -147,28 +147,54 @@ def _parser_with_string_args(_text: str) -> dict[str, Any]: "temperature": 0.75, } - def test_schema_coercion_skips_unknown_tools(self): - """If no matching tool schema exists, arguments should remain unchanged.""" + def test_a_call_naming_no_offered_tool_becomes_content(self): + """A call to a tool the caller never offered is delivered as content. - def _parser_with_string_id(_text: str) -> dict[str, Any]: - return { - "name": "process", - "arguments": {"action": "output", "id": "0"}, - } + Llama answers some plain questions with `<|python_tag|>print("hi")`, + which parses as a call to `print`. Handing the caller a tool name they + have no implementation for is worse than showing them the text. + """ - tools = [ + def _parser_calling_print(_text: str) -> dict[str, Any]: + return {"name": "print", "arguments": {"value": "hi"}} + + tools: list[dict[str, Any]] = [ { "type": "function", "function": { - "name": "different_tool", - "parameters": { - "type": "object", - "properties": {"id": {"type": "integer"}}, - }, + "name": "get_weather", + "parameters": {"type": "object", "properties": {}}, }, } ] + results = list( + parse_tool_calls( + _make_responses(["", "print", ""]), + make_mlx_parser("", "", _parser_calling_print), + tools, + ) + ) + + assert not any(isinstance(item, ToolCallResponse) for item in results) + text = "".join(getattr(item, "text", "") or "" for item in results if item) + assert "print" in text + + def test_schema_coercion_skips_tools_without_a_schema(self): + """If no matching tool schema exists, arguments should remain unchanged.""" + + def _parser_with_string_id(_text: str) -> dict[str, Any]: + return { + "name": "process", + "arguments": {"action": "output", "id": "0"}, + } + + # The tool is offered but declares no parameter schema, which is the + # case this covers. A call naming a tool that was never offered at all + # is a different matter and is dropped, see + # test_a_call_naming_no_offered_tool_becomes_content above. + tools = [{"type": "function", "function": {"name": "process"}}] + results = list( parse_tool_calls( _make_responses(["", "process", ""]), diff --git a/website/docs/api-guide.md b/website/docs/api-guide.md index 507588867..e9f4b4866 100644 --- a/website/docs/api-guide.md +++ b/website/docs/api-guide.md @@ -795,6 +795,34 @@ Typical flow: 4. Send the tool result back as a `tool` message. 5. Request the final model response. +Two behaviors are worth knowing when you send `tools`. + +**Only the tools you offer come back.** Some models reach for a built-in of +their own rather than one of yours: Llama answers some plain questions by +calling `print`, and gpt-oss has `python` and `browser`. A response that names +no tool you offered is returned as ordinary content with its normal +`finish_reason`, not as a `tool_calls` response, because you would have no +implementation to run. Check `finish_reason` rather than assuming a response is +a call. + +**`tool_choice` means the same thing on every engine.** `"none"` removes the +tools from the request, 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 you +asked for. A name matching none of your tools is left in the request rather +than silently emptying it, so the model answers from the full list; check the +returned call rather than assuming the name you forced. `"auto"` and `"required"` pass through, and +`"required"` is a best-effort instruction on the in-process engines rather than +a guarantee, because forcing a call there would need constrained decoding. + +**A JSON answer stays an answer.** Several model families write a tool call as +a bare JSON object, so a request that both offers tools and asks for JSON output +is ambiguous on the wire. Skulk resolves it in favor of the answer: text that +does not parse as a call to one of your tools is returned as content. Expect +that content to arrive in one piece rather than streamed token by token, since +it can only be classified once the message is complete. + ## Thinking / Reasoning Skulk supports reasoning-aware chat for compatible models. diff --git a/website/docs/architecture-reference.md b/website/docs/architecture-reference.md index 062eeea29..8057f731f 100644 --- a/website/docs/architecture-reference.md +++ b/website/docs/architecture-reference.md @@ -792,7 +792,99 @@ Inventory snapshot; see #130 for consolidation plan. | NemotronH | ~210 | `NemotronHShardingStrategy` + Mamba2 hybrid cache | | GPT-OSS | ~180 | MLX: `parse_gpt_oss` (token-level Harmony parser via `openai_harmony`) + `GptOssShardingStrategy`. llama.cpp: `HarmonyTextParser` in `harmony_text_parser.py` reparses the harmony channel markers from llama.cpp's detokenized *string* deltas (the engine exposes no token ids), splitting `analysis`→reasoning / `final`→content and stripping markers; wired in `llama_cpp/runner._generate`, gated on `OutputParserType.GptOss`, and dependency-free (no MLX/openai_harmony) so it runs on non-Mac GPU nodes. | | Step 3.5 | ~95 | Sliding-window cache tracking in `auto_parallel.py:639-650` | -| Llama / Ministral | ~70 | `LlamaShardingStrategy` (default) | +| Llama / Ministral | ~70 | `LlamaShardingStrategy` (default); the unmarked tool dialect (end-of-message stop token plus bare-object block opening, see "In-process tool-call dialects" below) in `utils_mlx.py` and `tool_parsers.make_text_dialect_parser` | + +## In-process tool-call dialects + +`worker/runner/llm_inference/tool_text_parser.parse_tool_calls_from_text` is +the shared dialect reader. The `llama_cpp` runner calls it directly for every +call its bundled chat handlers did not already parse. The `mlx` engine reaches +it only through the parsers wired onto the tokenizer in `utils_mlx`: the +generic `` dialect (`_parse_generic_text_tool_calls`) and the +unmarked dialect (`make_text_dialect_parser`) delegate to it, while a family +parser the tokenizer supplies itself is wrapped by `make_mlx_parser` and called +directly, and gpt-oss and DeepSeek V3.2 bypass it entirely for their own +token-level parsers (`parse_gpt_oss`, `parse_deepseek_v32`). Adding a dialect +here therefore reaches llama.cpp and those two MLX paths, not every MLX model. +Recognized dialects, tried in order: harmony `to=functions.NAME` channels +(gpt-oss); `` blocks carrying Hermes JSON, Qwen3 XML, or GLM +``/`` pairs; Llama `<|python_tag|>` calls (which use +`parameters` rather than `arguments` and may chain several with `;`); Mistral +`[TOOL_CALLS]` arrays; and an unmarked call object opening the message, which +the model may keep writing after. The unmarked rule is deliberately narrow, +since it is otherwise indistinguishable from a model answering in JSON: the +message must begin with the object, the object must carry a `name` alongside an +`arguments` or `parameters` value, and the offered-tools filter below removes +anything naming a tool the caller did not offer. The served engines (`llama_server`, `vllm`) do not use this +path: their servers parse tool calls themselves and return structured +`tool_calls`. + +The `mlx` engine drives this through a rolling marker scan +(`model_output_parsers.parse_tool_calls`). A generation chunk is whatever the +streaming detokenizer resolved that step, not a token, so a marker that is one +token id still arrives split (``). `_block_start_index` +therefore searches the accumulated text rather than each chunk, and +`_partial_marker_suffix_length` carries forward only the trailing run that +could still become a marker. That run is shorter than the longest marker, so +ordinary answers stream with at most a few characters of latency and nothing +is held for a message containing no call. The scan does not stop once ordinary +text has been released, so a model that writes a sentence before calling still +has its call found. A block closes at the first `end_parsing` found in the accumulated block, or at +the end of generation. The calls of every block in one message are coalesced +into a single `ToolCallResponse`, which is the OpenAI shape and is what makes +parallel calls survive: several families write each call in its own block, and +`API._token_chunk_stream` stops at the first chunk carrying a finish reason, so +a response per block would deliver the first call and drop the rest. Trailing +text is therefore released without its finish reason and the tool response is +the terminal chunk. The marker is located rather than matched at the end so +that text the model writes after the call ("`` Done.") returns to +the opening scan as ordinary text, where a second call in the same message is +still found. Reasoning chunks +(`is_thinking=True`) pass straight through and take no part in the scan, which +both keeps a thinking preamble from hiding the call and stops a call the model +only contemplated from being executed. + +Four properties on `ToolParser` carry the family differences: + +- `extra_start_parsing`: further markers that also open a block. Llama writes + the bare call object but prefixes `<|python_tag|>` when it names a tool, and + a marker that does not open the block reaches the caller as content. +- `anchored`: whether the primary marker opens a block only at the start of a + message. Set for the unmarked dialect, whose marker is `{`: distinctive + markers may open a block anywhere, but a brace also appears in prose and in + JSON answers. The families writing that dialect put the call in the whole + message, so anchoring costs nothing, and their `<|python_tag|>` marker still + opens a call after a preamble. +- `unparsed_is_text`: a block that fails to parse is content, not a failure. + Set for unmarked dialects, which open on `{` and therefore also catch a model + answering in JSON. +- `start_markers`: the read-side union of the primary and extra markers. + +`reject_unoffered_tool_calls` wraps `parse_gpt_oss` and `parse_deepseek_v32`, +which decode their calls from the token stream themselves and are selected +before the marker path, so they would otherwise bypass the rule below entirely; +a call it rejects is re-serialized as content. +`tool_parsers.declared_tool_calls` drops calls naming a tool the request did +not offer, and a block left with no offered tool is delivered as content. On +the llama.cpp path the same filter covers calls its bundled chat handlers +parsed natively (`offered_tool_calls_from_message`); since the handler consumes +the raw markup while parsing, a dropped native call is re-serialized as content +(`dropped_call_text`) rather than leaving a blank answer. This +is what keeps a model's own built-ins (Llama `print`, gpt-oss `python` / +`browser`) from reaching a caller that has no implementation for them. A +request that declared no tools is not parsed for calls at all: `apply_all_parsers` +skips the tool parser and the llama.cpp runner skips its recovery branch, since +the parser is wired from the tokenizer and cannot see what the request asked +for. `declared_tool_calls` itself treats a `None` tools list as "no list to +check against" rather than "nothing may be called", because the steward parses +its own turns through the same dialects without passing one. + +`utils_mlx.load_mlx_items` adds `<|eom_id|>` to `eos_token_ids` for any +tokenizer whose vocabulary has it. Llama declares only `<|eot_id|>`, so without +this the model generates past the end of its own tool call and emits the next +turn's header into the answer. Detection is by vocabulary, not by the chat +template mentioning the token: Llama's template routes tool results through the +`ipython` role and never writes `<|eom_id|>` or `<|python_tag|>` literally. ## KV cache backends diff --git a/website/docs/architecture.md b/website/docs/architecture.md index 5f1509e6b..4b52aa866 100644 --- a/website/docs/architecture.md +++ b/website/docs/architecture.md @@ -600,6 +600,30 @@ needs editing when Skulk catches up. Speech serving is the largest current example of that gating and has its own section below. +Model families do not agree on how a tool call is written, so the in-process +engines read the call out of the generated text with a shared set of dialects. +The llama.cpp runner uses that set for every call its own chat handlers did not +already parse; the MLX engine reaches it through two of the parsers it wires +onto a tokenizer, while a family parser the tokenizer supplies is used directly +and gpt-oss and DeepSeek keep their own token-level parsers. +Some families wrap the call in markers: a `` block carrying Hermes +JSON, Qwen3 XML, or GLM ``/`` pairs, a harmony +`to=functions.NAME` channel, or a Mistral `[TOOL_CALLS]` array. Llama uses no +opening marker at all: it writes the call object directly, sometimes prefixed +with `<|python_tag|>`, and ends the message with `<|eom_id|>` rather than a +closing marker. Skulk adds `<|eom_id|>` to the stop tokens for any model whose +vocabulary has it, because Llama declares only its end-of-turn token and +without that the model runs past the end of its own call and starts writing the +next turn. + +Two rules keep the unmarked case honest. A block that opens on `{` may just be +a model answering in JSON, so a block that does not parse as a call is +delivered as content rather than reported as a failure. And a call is only a +call if it names a tool the request offered: models reach for their own +built-ins (Llama answers some plain questions with a call to `print`, gpt-oss +has `python` and `browser`), and a caller has no implementation for those, so +those blocks come back as content too. + The llama.cpp runner serves GGUF models single-node and matches the MLX runner on the capabilities llama.cpp supports natively: per-token logprobs (with the top alternatives) and tool calling. A tool-enabled request runs unstreamed so diff --git a/website/docs/release-notes/next.md b/website/docs/release-notes/next.md index d5ce55a52..4c1f9723c 100644 --- a/website/docs/release-notes/next.md +++ b/website/docs/release-notes/next.md @@ -4,6 +4,50 @@ title: Next release sidebar_position: 0 --- +## Tool calling across more model families + +A model that calls several tools at once now returns all of them. Models that +write each call separately previously came back with only the first. + + +A model that reasons before calling a tool now has its call recognized. Its +reasoning previously made the request look like an ordinary answer, so the call +that followed was returned as raw markup in the message content. A call a model +only thought about while reasoning is also no longer carried out. + + +The `tool_choice` option now behaves the same way whichever engine serves the +model. It previously reached only the engines that run an inference server of +their own, so a request that sent `"none"` could still come back with a tool +call. Sending `"none"` now guarantees no call, and naming a single function +guarantees the model cannot call a different one. + + +Tool calls are now recognized when a model's opening marker arrives split +across several streamed pieces, which is the normal case rather than the +exception, and when the model writes a sentence before calling ("I'll check +that.") rather than opening with the call. Previously the caller received the +raw markup as message content with an ordinary stop reason, so a well formed +call from the model looked like a refusal to call anything. + + +Tool calling now works for Llama models served by the MLX engine, and Skulk +recognizes the ways more model families write a call. Llama ends a message that +hands off to a tool with a token it does not declare as a stop token, so +generation used to continue past the end of the call and write the next turn's +opening into the answer. It also writes the call as a plain JSON object with no +opening marker, which nothing recognized as a call, so a request that offered a +tool came back with JSON in the message content and no tool call at all. Both +are fixed, and the recognized formats now cover Llama calls, Mistral tool-call +arrays, and GLM argument pairs alongside the formats already supported. + +Two rules make the result predictable when you send tools. A response that +names no tool you offered comes back as ordinary content rather than as a tool +call, because models sometimes reach for a built-in of their own that you have +no implementation for. And 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 +available, is returned as content rather than reported as a generation error. + ## Exact artifact bundles Signed registry-v2 cards may now identify one complete executable artifact or