diff --git a/cmd_chat/agent/providers.py b/cmd_chat/agent/providers.py index 5fa9533..a0f07bb 100644 --- a/cmd_chat/agent/providers.py +++ b/cmd_chat/agent/providers.py @@ -11,6 +11,7 @@ from __future__ import annotations import importlib import json import os +import re from dataclasses import dataclass from typing import Protocol, runtime_checkable @@ -164,56 +165,104 @@ class OllamaProvider: args = {} calls.append({"name": fn.get("name", ""), "arguments": args or {}}) # Small/quantized models (notably qwen2.5 on CPU) intermittently emit a valid - # tool call as literal `{…}` text in `content` instead - # of the structured `tool_calls` field. Recover those so a correct action - # isn't silently dropped (and strip the tags from the chat-facing text). - if not calls and "" in text: - text, calls = self._extract_text_tool_calls(text) + # tool call as literal text in `content` instead of the structured `tool_calls` + # field — qwen's `{…}`, but also bare/fenced JSON and + # alternate wrappers (``, ``). This is the single biggest + # score sink in the native-harness benchmark, so recover any well-formed JSON + # call here. Gate on the known tool names from `tools` so a stray JSON blob in + # prose can never be coerced into an action the model didn't structurally ask + # for. Only adopt the recovery when it actually found a call (a plain `DONE:` + # or prose turn is left untouched). + if not calls and text: + valid = {(t.get("function") or {}).get("name") for t in (tools or [])} + valid.discard(None) + recovered_text, recovered = self._extract_text_tool_calls(text, valid) + if recovered: + text, calls = recovered_text, recovered return text, calls - @staticmethod - def _extract_text_tool_calls(text: str) -> tuple[str, list[dict]]: - """Pull `{json}` blocks out of model text (qwen's - text-mode tool calls). Uses a JSON decoder (not regex) so nested braces in - arguments parse correctly; tolerates a missing closing tag. Returns the text - with the blocks removed and the recovered calls.""" + # Wrapper tags a weak model wraps a leaked call (or its prose) in; stripped + # from the chat-facing text once the JSON inside is recovered. + _WRAP_TAGS = re.compile( + r"", + re.I, + ) + + @classmethod + def _coerce_call(cls, obj, valid_names) -> dict | None: + """Turn a decoded JSON object into a `{"name","arguments"}` call IF it + structurally is one for a KNOWN tool — else None. Unwraps the OpenAI-style + `{"function": {...}}` / `{"tool_call": {...}}` nesting and accepts either + `arguments` or qwen's `parameters` key. The `valid_names` gate is what makes + scanning arbitrary text safe: a random JSON blob in prose has no known tool + name, so it can never be coerced into an action.""" + if not isinstance(obj, dict): + return None + inner = obj.get("function") or obj.get("tool_call") + if isinstance(inner, dict): + obj = inner + name = obj.get("name") + if not isinstance(name, str) or not name: + return None + if valid_names and name not in valid_names: + return None + args = obj.get("arguments") + if args is None: + args = obj.get("parameters") + if isinstance(args, str): + try: + args = json.loads(args) + except ValueError: + args = {} + if not isinstance(args, dict): + args = {} + return {"name": name, "arguments": args} + + @classmethod + def _extract_text_tool_calls( + cls, text: str, valid_names: set | None = None + ) -> tuple[str, list[dict]]: + """Recover tool calls a small/quantized model emitted as TEXT in `content` + instead of the structured `tool_calls` field. Handles qwen's + `{json}` blocks plus the looser CPU-model leaks: bare + JSON, ```json fenced blocks, and alternate wrapper tags (``, + ``). Scans for every JSON object via a decoder (so nested + braces in arguments parse correctly) and keeps ONLY those that `_coerce_call` + accepts as a known tool — never freeform prose, so it can't fabricate an + action the model didn't structurally request. Returns the text with the + recovered JSON (and now-orphaned wrapper tags / code fences) stripped, plus + the calls.""" dec = json.JSONDecoder() calls: list[dict] = [] spans: list[tuple[int, int]] = [] - idx = 0 - while True: - tag = text.find("", idx) - if tag == -1: - break - brace = text.find("{", tag) + i, n = 0, len(text) + while i < n: + brace = text.find("{", i) if brace == -1: break try: obj, end = dec.raw_decode(text, brace) except ValueError: - idx = tag + len("") + i = brace + 1 continue - if isinstance(obj, dict) and obj.get("name"): - args = obj.get("arguments") - if args is None: - args = obj.get("parameters") - if isinstance(args, str): - try: - args = json.loads(args) - except ValueError: - args = {} - calls.append({"name": obj["name"], "arguments": args or {}}) - close = text.find("", end) - span_end = close + len("") if close != -1 else end - spans.append((tag, span_end)) - idx = span_end + call = cls._coerce_call(obj, valid_names) + if call is not None: + calls.append(call) + spans.append((brace, end)) + i = end if spans: kept, last = [], 0 for start, stop in spans: kept.append(text[last:start]) last = stop kept.append(text[last:]) - text = "".join(kept).strip() + text = "".join(kept) + # The JSON is gone; drop the wrapper tags and any now-empty code fences + # it sat in so the chat summary reads as clean prose. + text = cls._WRAP_TAGS.sub("", text) + text = re.sub(r"```[a-zA-Z]*\s*```", "", text) + text = re.sub(r"```[a-zA-Z]*|```", "", text) + text = text.strip() return text, calls def stream(self, system: str, messages: list[Msg]): diff --git a/docs/findings-native-harness-2026-06-10.md b/docs/findings-native-harness-2026-06-10.md index fcc964c..dbe4334 100644 --- a/docs/findings-native-harness-2026-06-10.md +++ b/docs/findings-native-harness-2026-06-10.md @@ -214,9 +214,52 @@ Dominant failure modes the summaries exposed, and where they point next: | path hallucination | `/home/qwen253b/conf_count.txt`, `~/` unexpanded | partly (model) | | content fabrication | wrote literal `dell` instead of running whoami | no (model capability) | -The first row is the clear top priority — it is the single biggest score sink across -all four models, and it is purely a harness parse gap. The benchmark now exists to -prove whether fixing it moves the number. +The first row was hypothesised as the top priority — a pure harness parse gap. The +benchmark now exists to prove whether fixing it moves the number. + +### Parser fix — what the benchmark actually proved (2026-06-10) + +Generalised `OllamaProvider._extract_text_tool_calls` to recover a tool-call JSON +object regardless of wrapper — qwen's `` tags, bare JSON, ```json fences, +alternate tags (``, ``), OpenAI `{"function":{…}}` nesting, and +`parameters`-vs-`arguments`. Gated on the known tool-name set (derived from the +`tools` schema) so a stray JSON blob in prose — or a hallucinated `make_dir` — can +never be coerced into an action. 11-case unit check: 8 leak shapes recover, 3 +negatives (prose / unknown tool / random config JSON) ignored. + +**Result: it does NOT move the weak-model pass rate.** Three 3b passes went 2 / 1 / 0 +of 12 (prior baseline 1/12 — pure noise); two 0.5b passes went 0 / 0 (prior 1/12). + +A direct `/api/chat` probe of 0.5b shows *why* — the hypothesis was wrong about the +**form** of the leak. The weak models do not emit a JSON tool-call as text. They emit +either: + + * **malformed structured `tool_calls`** — e.g. `write_file` with `content: null` + (the field IS populated; the arguments are just wrong → model capability, not a + parse gap); or + * **a fenced shell/code block in prose with NO tool call at all** — e.g. content + `"…let's proceed:\n```bash\nmkdir -p a/b/c\n```"`, `tool_calls: None`. This is the + "[stopped — model described the task but never ran a tool]" case that dominates + the 0.5b table. + +So the structured-JSON recovery is a correct, safe hardening (it WILL catch a real +JSON-in-text leak from any model that produces one, and cannot fabricate an action), +but the failure it was meant to fix is not the failure these CPU models actually have. + +**The real harness-addressable lever the data now points to:** recover **fenced code +blocks** (```bash … ``` / ```python … ```) as `run_shell` calls when the turn carried +no structured call. That is the genuine form of the weak-model leak. It is a bigger +safety call than JSON recovery (it executes a block the model wrote as prose, which +may be illustrative rather than an action), so it is left as an explicit decision, not +folded in silently. + +| failure mode | example | harness-addressable? | +|---|---|---| +| malformed structured args | `write_file` with `content:null` | no (model capability) | +| fenced code in prose, no call | ```` ```bash\nmkdir -p a/b/c\n``` ````, `tool_calls:None` | **yes, but a safety call** — parse fences → run_shell | +| JSON tool-call leaked as text | `{"name":…}` | yes — **now handled** (no weak-model effect) | +| path hallucination | `/home/qwen253b/…`, `/ai/a/b/c`, `~/` unexpanded | partly (model) | +| content fabrication | wrote literal `octocat`/`dell` instead of running whoami | no (model capability) | **Other models worth pulling for a non-qwen data point** (different family → different failure modes, both with strong native tool-calling): `llama3.2:3b` (~2 GB, peer to