diff --git a/CLAUDE.md b/CLAUDE.md index b9d8e26..75c9618 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -289,7 +289,14 @@ so failing early is better than an opaque provider 400. ### Local Ollama vision fallback `local_vision:` in `config.yaml` configures a fallback path for image requests -that find no cloud vision candidate. It ships **disabled** (`enabled: false`). +that find no cloud vision candidate — the cloud catalog excludes the cost +leader (deepseek) on vision, so without this fallback every image request +that would otherwise have routed there 422s instead. It is a **core feature**, +**enabled by default** (`enabled: true`, both in `config.yaml` and in +`LocalVisionConfig`'s own default, so a config that omits the section still +gets it). Disable it explicitly (`enabled: false`) on a host with no local +Ollama, or one that hasn't pulled the vision model. + When enabled and routing returns no selected model, `_run_local_vision` sends the original messages, with `image_url` parts intact, to a local Ollama model (`qwen3-vl:4b` by default). The local answer then **replaces** the completion: @@ -1007,6 +1014,12 @@ match `classifier.model` in `config.yaml`: ollama pull mistral-nemo:12b ``` +`local_vision` ships **enabled** as a core feature (see below), so also pull +its model unless you're turning it off: +```bash +ollama pull qwen3-vl:4b +``` + It does not have to be on this machine. To use one across a VPN, point `classifier.base_url` and `verification.base_url` at it and apply `deploy/ollama-over-vpn.conf` on the serving host — Ollama binds loopback-only diff --git a/README.md b/README.md index c5bb3b3..3d75a2e 100644 --- a/README.md +++ b/README.md @@ -373,7 +373,7 @@ failure — it means the checker had nothing to say, not that the model failed. ## Weighted Scoring -Four hard filters are applied **before** scoring (not weighted — outright disqualification): +Six hard filters are applied **before** scoring (not weighted — outright disqualification): 1. `effective_context_window ≥ required_context_tokens` 2. `tier ≥ required_tier` (from classifier) diff --git a/capabilities.py b/capabilities.py index d25c750..ce5057b 100644 --- a/capabilities.py +++ b/capabilities.py @@ -19,6 +19,7 @@ recorded so dispatcher can observe it, but never fed to a routing filter. from __future__ import annotations +from collections.abc import Iterator from dataclasses import dataclass from typing import Any @@ -68,7 +69,18 @@ def detect_capabilities(body: dict[str, Any]) -> RequestCapabilities: ) -def _any_message_has_images(messages: list[Any]) -> bool: +def iter_image_url_values(messages: list[Any]) -> Iterator[str]: + """Every image_url part's URL/data-URI string, across all messages. + + Yields exactly one string per image_url-typed part, even when no URL can + be resolved from it (empty string). A part that declares itself an image + but carries no readable value must still count toward "this conversation + has an image" and still fail the "every part is a safe data: URI" check + -- silently skipping it, as an earlier version of this function did, + dropped it out of both checks: a degenerate part could then dodge the + vision-capability gate AND slip past the SSRF guard that only holds when + every yielded value is verified to start with ``data:``. + """ for message in messages: if not isinstance(message, dict): continue @@ -76,6 +88,16 @@ def _any_message_has_images(messages: list[Any]) -> bool: if not isinstance(content, list): continue for part in content: - if isinstance(part, dict) and part.get("type") == "image_url": - return True - return False + if not isinstance(part, dict) or part.get("type") != "image_url": + continue + value = part.get("image_url") + if isinstance(value, dict): + value = value.get("url", "") + if not isinstance(value, str): + # Bare url key (alternative shape), or nothing resolvable. + value = part.get("url") if isinstance(part.get("url"), str) else "" + yield value + + +def _any_message_has_images(messages: list[Any]) -> bool: + return any(True for _ in iter_image_url_values(messages)) diff --git a/code_plans/shared-helper-unparseable-input-contract.md b/code_plans/shared-helper-unparseable-input-contract.md new file mode 100644 index 0000000..5c17c6c --- /dev/null +++ b/code_plans/shared-helper-unparseable-input-contract.md @@ -0,0 +1,140 @@ +# Spec + implementation plan: pin down what a shared helper yields on well-typed-but-unparseable input + +**Origin.** PR #3 collapsed four independent `messages` → `image_url` part +traversals into one generator, `capabilities.iter_image_url_values` +(`code_reviews/capability-gate-followups.md`, item #6). The plan specified +the two shapes the four originals agreed on (`image_url` as a dict with +`url`, or as a bare string) but not the case where a part is typed +`image_url` and carries neither — well-typed, but nothing to extract. The +merged helper silently skipped that case instead of yielding a placeholder, +which dropped a degenerate part out of two fail-closed checks at once (see +`code_reviews/pr-3-capability-gate-followups-review.md` for the live +repro and the fix). The follow-up opencode session that implemented the fix +named the general lesson explicitly, which is what this document is +formalizing: **a plan that proposes collapsing N implementations into one +shared helper has to specify the helper's contract on every input shape, +not only the ones the N originals happened to agree on.** + +This is a process document, not a bug report — no code changes accompany +it. It exists so the next plan that proposes a shared helper pins this down +up front instead of it being rediscovered in review. + +--- + +## Spec + +### The problem, generalized + +When several independent implementations of "walk this structure and pull +out X" get unified into one shared helper, each original's handling of its +*own* edge cases has to be re-derived by the person writing the unification +— nothing forces them to enumerate it, because the edge case was never the +reason any of the N implementations were written in the first place. Each +one just happened to fall into some fallback path. The happy path is easy +to unify because all N agree on it by construction (they all worked, on +real traffic, for the common shape). The edge cases are exactly where they +are most likely to *quietly disagree*, and disagreement there is invisible +unless a plan makes each implementation's edge-case behavior explicit +before proposing the replacement. + +The specific edge case that bit PR #3 — matches the coarse type tag but the +payload is missing or empty — deserves its own name because it is a +distinct category from "wrong type" or "absent": it is a **well-typed but +unparseable** input. A traversal that filters on `part.get("type") == +"image_url"` before doing anything else will always let this shape through +the filter; what happens next is the part that has to be decided +deliberately. + +### The rule + +Any implementation plan that proposes extracting or unifying a shared +traversal, parser, or extractor helper from multiple existing call sites +must include an explicit contract: for every input shape the helper will +see, state what it yields or returns. At minimum, enumerate: + +1. **The common/valid shape** — the one all N originals handle the same way. +2. **Every alternate valid shape** any existing call site already handles + (there may be more than one — PR #3 had three: `image_url` as + `{"url": ...}`, `image_url` as a bare string, and a bare `url` key on + the part itself). +3. **The well-typed-but-unparseable shape** — passes the coarse type/shape + check but the value cannot actually be extracted. State the yielded + value explicitly (a sentinel like `""`, not "nothing" / "skip") unless + skipping is a deliberate, stated decision. +4. **The wrong-typed or absent shape** — fails the coarse check. Usually + "skip", but say so, so it's a decision and not an assumption. + +And critically: **for category 3, list what each of the N originals +currently does**, not just what the new helper will do. That list is what +surfaces disagreement before the merge, rather than after. In PR #3, all +four originals agreed by accident (each one's "nothing extracted" path +happened to either count the part or fail closed) — but nothing in the +plan recorded that agreement, so the merge didn't have to preserve it and +didn't. + +### Why "skip silently" is the default failure mode to watch for + +A `for`/`if`/`continue` traversal skips by construction unless a branch +explicitly appends/yields on every path. Converting four such loops into +one `yield`-per-match generator is a natural, idiomatic simplification — +and it's exactly the transformation that turns "always contributes +*something*, even a fail-closed placeholder" into "contributes nothing, +silently, for a whole category of input." This isn't specific to +generators — the same risk applies to a shared parser that returns `None` +for the unparseable case where callers previously each had their own +`None`-handling that didn't all agree — but generators make it easy to +introduce, because `continue`-without-yielding reads as the most natural +thing to write. + +--- + +## Implementation plan + +This is a discipline to apply going forward, not a code change. Concretely: + +### 1. Template for future plans + +When a `code_plans/` (or `code_reviews/`) entry proposes extracting a +shared helper from multiple call sites, include a table shaped like this +alongside the proposed code: + +| Input shape | Original A | Original B | Original C | ... | New shared helper | +|---|---|---|---|---|---| +| common/valid | ... | ... | ... | | ... | +| alternate valid #1 | ... | ... | ... | | ... | +| well-typed, unparseable | ... | ... | ... | | ... (state the sentinel) | +| wrong-typed / absent | skip | skip | skip | | skip | + +If any two originals disagree in a row, that disagreement is the thing the +plan has to resolve on purpose (pick one, document why) rather than let the +merge resolve by accident. + +### 2. No retroactive audit needed right now, but a watch list for next time + +Checked the rest of `dispatcher.py` for other traversals of a message's +multimodal `content` list, since that's the same shape that bit PR #3. +None of these are currently merged/unified helpers, so none are broken — +they're independent, single-purpose traversals, not N-collapsed-into-1. But +if any of them are ever unified (with each other, or with +`iter_image_url_values`), this spec applies: + +- `session_fingerprint` (dispatcher.py:1302) — joins text parts to fingerprint a session. +- `session_directory` (dispatcher.py:1321) — joins text parts to find a working directory. +- `estimate_prompt_tokens` (dispatcher.py:1364) — sums text-part lengths for the context estimate. +- `_last_user_text` (dispatcher.py:1391) — joins the last user turn's text parts. + +All four already treat a non-dict or non-string part as "contributes +nothing" consistently with each other (each filters with +`isinstance(p, dict)` / `isinstance(p.get("text"), str)` before including +it), so there's no live disagreement to flag today — this is a watch list, +not a finding. + +### 3. Where this lives + +Keep this document as the reference the next relevant plan cites, rather +than duplicating the rule inline each time. A future plan proposing a +shared-helper extraction should link back to this file +(`code_plans/shared-helper-unparseable-input-contract.md`) and fill in the +contract table from §1 as part of the plan, the same way +`code_reviews/capability-gate-followups.md` gave concrete before/after code +for each finding rather than describing it in prose only. diff --git a/code_reviews/capability-gate-followups.md b/code_reviews/capability-gate-followups.md new file mode 100644 index 0000000..84318e6 --- /dev/null +++ b/code_reviews/capability-gate-followups.md @@ -0,0 +1,319 @@ +# Capability-gate follow-ups + +Source: `/code-review medium` on `neuralwatt-router-service`, 2026-08-23, run +against the capability-gate / local-vision-fallback work +(`4ce9959`..`c4ebc4b`..`cff36f8`). 8 findings came back; 2 are fixed +(`69d3a8b`: `local_vision.enabled` now ships/defaults `true` as a documented +core feature, and the `provider/model` prefix is now resolved against the +catalog for a pin, not only for `auto`). This tracks the remaining 6. + +No timeline attached — pick items up in any order except where a dependency +is called out below. #3 and #6 touch the same two functions and share a +root cause, so doing them together avoids editing the same lines twice. + +--- + +## 1. Local vision fallback ignores `require_json_mode` + +**File:** `dispatcher.py`, fallback trigger at `chat_completions` (~line +1785-1794); the check function is `_run_local_vision` (~line 1478). + +**Problem.** The fallback fires on `caps.has_images and +cfg.local_vision.enabled` alone: + +```python +if decision.selected is None: + if ( + caps.has_images + and cfg.local_vision.enabled + ): + fallback = _run_local_vision(messages, cfg.local_vision) +``` + +It never checks `caps.require_json_mode`. If a request carries both an +image and `response_format: {type: json_object}`, and JSON mode — not +vision — is what excluded every candidate, the fallback still fires. +`_local_vision_response` wraps the local model's free-form text as a normal +completion with no JSON-mode enforcement, silently breaking the caller's +`json_object` contract and returning 200 instead of the 422 that would have +correctly named the missing capability. + +**Fix.** Local vision only knows how to answer with prose, so it must not +be attempted when JSON mode was requested: + +```python +if decision.selected is None: + if ( + caps.has_images + and not caps.require_json_mode + and cfg.local_vision.enabled + ): + fallback = _run_local_vision(messages, cfg.local_vision) +``` + +When both are true, this now falls through to the existing 422, which +already lists `caps.require_json_mode` in `limits` (line ~1813-1816) — no +change needed there. + +**Test to add** (`tests/test_chat_completions.py`, alongside +`test_local_fallback_refuses_a_remote_image_url` and the other +`local_vision` tests): an image + `response_format: json_object` request, +with `local_vision.enabled = True` and no cloud vision candidate, must +still 422 and must not reach the fake local-vision `fake_post`. + +--- + +## 2. Pinned-model capability gate ignores `require_vision`/`require_json_mode` config + +**File:** `dispatcher.py`, `_check_pinned_capabilities` (line 1622) and its +call site (line 1844-1845). + +**Problem.** `route()` only applies the vision/json-mode filter when the +corresponding config flag is on: + +```python +require_vision=cfg.routing.require_vision if req.has_images else False, +require_json_mode=( + cfg.routing.require_json_mode if req.require_json_mode else False +), +``` +(`dispatcher.py` ~line 610-613, feeding `routing.rejection_reason`.) + +`_check_pinned_capabilities` has no such condition — it checks +`caps.has_images` / `caps.require_json_mode` directly, with no reference to +`cfg.routing.require_vision` / `require_json_mode` at all. If an operator +sets `routing.require_vision: false` (accepting the occasional provider +400 in exchange for not fail-closing on an unconfirmed flag), `auto` +traffic honors that, but a pin with an image still gets an unconditional +422. Same request shape, different outcome, depending only on whether the +client said `auto` or a real model id. + +**Fix.** Covered by the refactor in #6 below — once +`_check_pinned_capabilities` calls the same shared helper `route()` uses, +threading the two config flags through is one line each. If #6 is deferred, +the standalone fix is: + +```python +if cfg.routing.require_vision and caps.has_images and (row is None or row["supports_vision"] != 1): + ... +if cfg.routing.require_json_mode and caps.require_json_mode and (row is None or row["supports_json_mode"] != 1): + ... +``` + +**Test to add:** with `cfg.routing.require_vision` monkeypatched `False`, +a pin to a non-vision model with an image must dispatch (200), not 422. + +--- + +## 3. `opencode.json` advertises image support `deepseek-v4-flash` doesn't have + +**File:** `opencode.json`, the `deepseek-v4-flash` model entry (line ~39-51). + +**Problem.** Confirmed against the live catalog: + +``` +$ sqlite3 router.db "SELECT model_id, supports_vision FROM models WHERE model_id='deepseek-v4-flash';" +deepseek-v4-flash|0 +``` + +but the opencode entry declares: +```json +"deepseek-v4-flash": { + "modalities": { "input": ["text", "image"] } +} +``` +copied from the other pins, all of which really do support vision +(`gemma-4-31b`, `kimi-k2.7-code`, `kimi-k3`, `qwen3.6-35b` all read +`supports_vision = 1`). opencode's UI trusts this and lets a user attach an +image to the deepseek pin; the request then either 422s (once #2 above is +fixed) or, today, gets forwarded and fails some other way — either way it's +a late, confusing failure on a model the client's own config advertised as +supporting images, instead of the image-attach control being disabled for +that one pin. + +**Fix.** Drop the `modalities` block from the `deepseek-v4-flash` entry (or +set `"input": ["text"]`). No code change; this is a static config edit. + +**Verification.** Since the catalog's vision support can change if +NeuralWatt updates the model, this is a fact worth re-checking rather than +assuming — re-run the query above before editing if it's been a while, and +consider whether `poller.py` should be the source of truth for +`opencode.json`'s modality lists instead of a hand-maintained copy (out of +scope here; noting it so it doesn't get re-discovered from scratch next +time this file needs an update). + +--- + +## 4. README's "Four hard filters" heading undercounts + +**File:** `README.md`, line 376. + +**Problem.** +``` +Four hard filters are applied **before** scoring (not weighted — outright disqualification): +``` +immediately precedes a 6-item list (context, tier, latency/access, +tool-proficiency, vision, json-mode), and the paragraph right after it +already says "Filters 4–6" (line 388). Leftover from the vision/json-mode +filters this diff added without updating the count above them. + +**Fix.** One-word change: `Four` → `Six`. + +--- + +## 5. `_check_pinned_capabilities` duplicates the centralized gate logic + +**File:** `dispatcher.py` (`_check_pinned_capabilities`, line 1622) vs. +`routing.py` (`rejection_reason`, line 36, vision/json-mode block at +line 104-121). + +**Problem.** `routing.rejection_reason` already centralizes the fail-closed +vision/json-mode rule (unknown flag → reject, `False` flag → reject, `True` +→ pass), used by every routed (`auto`) request. `_check_pinned_capabilities` +re-implements the same rule with its own raw SQL and a hardcoded pair of +`if`s for exactly these two flags. Two independent copies of one rule is +how #2 above happened, and it's how a third gated capability (audio, say) +would have to be added in `capabilities.py`, `routing.py` (the dataclass +*and* `rejection_reason`), `TaskRequest`, both `route()` call sites, +`config.py`/`config.yaml`, *and* a new `if` block here — six-plus places +for one flag, free to drift. + +**Fix.** Extract the capability-flag check out of `rejection_reason` into +its own function in `routing.py`, since it's the one self-contained piece +of that rule (unlike context/tier/tool-proficiency, a pin deliberately +skips those — "a client pinned to one model gets none of the filtering or +ranking below" is intentional, so this must NOT become "call +`rejection_reason` for pins too"): + +```python +# routing.py +def capability_gate_reason( + row: dict, *, require_vision: bool = False, require_json_mode: bool = False, +) -> str | None: + """Same fail-closed rule `rejection_reason` uses for vision/json-mode, + pulled out so a pinned-model check can reuse it without re-implementing + it — see dispatcher._check_pinned_capabilities.""" + if require_vision: + vision = row.get("supports_vision") + if vision is None: + return "vision(unknown)" + if not vision: + return "vision(unsupported)" + if require_json_mode: + jm = row.get("supports_json_mode") + if jm is None: + return "json_mode(unknown)" + if not jm: + return "json_mode(unsupported)" + return None +``` + +`rejection_reason` calls it in place of its inline block (behavior +unchanged — same checks, same order, same reason strings, so +`tests/test_routing.py`'s existing assertions on those strings still hold). + +`_check_pinned_capabilities` becomes: + +```python +def _check_pinned_capabilities(model_id: str, caps) -> None: + conn = _db() + try: + row = conn.execute( + "SELECT supports_vision, supports_json_mode FROM models " + "WHERE model_id = ? AND provider = 'neuralwatt'", + (model_id,), + ).fetchone() + finally: + conn.close() + reason = capability_gate_reason( + dict(row) if row else {}, + require_vision=cfg.routing.require_vision and caps.has_images, + require_json_mode=cfg.routing.require_json_mode and caps.require_json_mode, + ) + if reason is not None: + capability = "vision" if reason.startswith("vision") else "json mode" + raise HTTPException(422, f"{model_id} does not support {capability}...") +``` + +This is the same refactor that fixes #2 (config-flag conditioning) — do +them as one change, not two. + +**Test to add:** `tests/test_routing.py` already covers +`rejection_reason`'s vision/json-mode branches; add a direct unit test for +`capability_gate_reason` (or just keep testing it through +`rejection_reason`, since that's now a thin wrapper — either is fine, pick +whichever the existing test file's style favors). The existing +`test_a_pinned_non_vision_model_with_an_image_is_a_clear_422` and the two +prefix-resolution tests added in `69d3a8b` should keep passing unmodified; +that's the regression check that the refactor didn't change behavior for +the default (`require_vision: true`) config. + +--- + +## 6. Image-part traversal reimplemented four times + +**Files:** `dispatcher.py` — `_count_images` (line 1405), +`_image_payload_bytes` (line 1424), `_local_vision_data_uris_ok` +(line 1450); `capabilities.py` — `_any_message_has_images` (line 71). + +**Problem.** All four walk the same shape (`messages` → dict with a +`content` list → parts where `part["type"] == "image_url"`) with slightly +different unwrapping of the `image_url` value (dict-with-`url`-key vs. +bare string) between them. A future change to how images are represented +in the request body has to be applied in four places; missing one silently +breaks detection, the byte budget, or the SSRF data-URI guard while the +others keep working. `_run_local_vision` also calls `_count_images` and +`_image_payload_bytes` twice each — once for the budget check, again to +log on the failure path — doubling the scan for no reason. + +**Fix.** One shared generator that yields each image part once, with the +`image_url` value already unwrapped to a string: + +```python +# capabilities.py, since dispatcher.py already imports from it +def iter_image_url_values(messages: list[Any]) -> Iterator[str]: + """Every image_url part's URL/data-URI string, across all messages.""" + for message in messages: + if not isinstance(message, dict): + continue + content = message.get("content") + if not isinstance(content, list): + continue + for part in content: + if not isinstance(part, dict) or part.get("type") != "image_url": + continue + value = part.get("image_url") + if isinstance(value, dict): + value = value.get("url", "") + if isinstance(value, str): + yield value +``` + +Then: +- `_any_message_has_images` → `any(True for _ in iter_image_url_values(messages))` +- `_count_images` → `sum(1 for _ in iter_image_url_values(messages))` +- `_image_payload_bytes` → `sum(len(v) for v in iter_image_url_values(messages))` +- `_local_vision_data_uris_ok` → `all(v.startswith("data:") for v in iter_image_url_values(messages))` + +And in `_run_local_vision`, compute `_count_images(messages)` and +`_image_payload_bytes(messages)` once each and reuse the value in both the +condition and the log call, rather than recomputing for the log line. + +**Test to add:** none new — this is a pure refactor behind the same four +call sites, all of which already have test coverage +(`test_local_fallback_refuses_a_remote_image_url`, +`test_the_two_pass_reroute_passes_image_capability`, and the capability +tests in `tests/test_capabilities.py`). Running the existing suite green is +the verification. + +--- + +## Suggested order + +1. **#1** (json-mode-blind fallback) — standalone, quick, clear correctness fix. +2. **#5 + #2 together** (extract `capability_gate_reason`, thread config + flags through the pinned check) — one refactor fixes both. +3. **#6** (shared image-part iterator) — touches the same file as #2/#5; + convenient to do in the same sitting but not dependent on it. +4. **#3** (`opencode.json` modality) and **#4** (README count) — trivial, + do whenever, in any order. diff --git a/code_reviews/pr-3-capability-gate-followups-review.md b/code_reviews/pr-3-capability-gate-followups-review.md new file mode 100644 index 0000000..742b31a --- /dev/null +++ b/code_reviews/pr-3-capability-gate-followups-review.md @@ -0,0 +1,84 @@ +# Review: PR #3, "fix: resolve remaining capability-gate review findings" + +**What it was reviewing:** the implementation of +[`capability-gate-followups.md`](capability-gate-followups.md), done by +opencode driven through the router itself (`llm-router`) rather than by a +person. Reviewed by re-reading the diff against the plan, and by running +the suite locally. + +## Verdict: all 6 items done correctly; one gap in a part it added beyond the plan + +Each plan item was implemented as specified, and matches the concrete code +the plan proposed closely enough that it reads as if it were typed from it: + +| # | Item | Verified | +|---|---|---| +| 1 | Local vision fallback honors `require_json_mode` | Fix is the exact one-line change the plan proposed; test posts an image + `json_object` request and asserts the local fallback's `fake_post` is never called. | +| 2 | Pinned gate honors `cfg.routing.require_vision`/`require_json_mode` | Threaded through `_check_pinned_capabilities`; test flips `require_vision` off and asserts a non-vision pin now dispatches (200) instead of 422. | +| 3 | `opencode.json` deepseek-v4-flash modality | `image` dropped from the entry; re-checked live against `router.db` — `supports_vision = 0` for `deepseek-v4-flash`, confirming the edit is correct, not just plan-compliant. | +| 4 | README "Four hard filters" → "Six" | Matches. | +| 5 | Extract `capability_gate_reason` | `rejection_reason` now delegates to it; `_check_pinned_capabilities` reuses it via `dict(row)` (correctly worked around `sqlite3.Row` having no `.get()`). 6 new unit tests cover every branch of the extracted function directly. | +| 6 | Single `iter_image_url_values` generator | All four traversal sites collapsed onto it; `_run_local_vision` now computes count/bytes once each instead of twice. | + +Full suite after the PR's own changes: 416 passed (matches the PR +description's own count). + +## The gap: a fail-open regression in `iter_image_url_values`, introduced beyond the plan + +The plan's sketch for `iter_image_url_values` only handled the two shapes +the four original hand-rolled loops handled (`image_url` as a dict with +`url`, or as a bare string). The merged version added a third branch — +`elif isinstance(part.get("url"), str))`, a bare `url` key at the part +level — which is legitimate: it's required to keep a **pre-existing** test +(`test_image_url_part_sets_has_images`, not touched by this PR) passing, +since that fixture uses exactly that shape. + +But the three-way `if/elif` only yields when one of the three shapes +resolves. A part that is `{"type": "image_url"}` with **none** of them — +spec-invalid, but the four original implementations each handled it +explicitly — now yields nothing at all, and silently drops out of every +derived check. Confirmed live, before the fix in this pass: + +```python +msgs = [{"role": "user", "content": [{"type": "image_url"}]}] +# before: has_images=False, data_uris_ok=True (both wrong) +# after: has_images=True, data_uris_ok=False (matches pre-PR behavior) +``` + +Two consequences, both regressions from the pre-PR code: +- `_any_message_has_images` → `False`: the request could route to a + non-vision model instead of being gated, the exact failure mode + `require_vision` fail-closed-on-unknown exists to prevent. +- `_local_vision_data_uris_ok` → `True`: the SSRF guard (every image part + must be a verified `data:` URI before the local model is trusted with it) + passed vacuously, since `all()` over zero yielded items is `True`. + +No test — old or new — exercised this shape either way, which is why it +shipped. + +## Fix applied in this pass + +`capabilities.iter_image_url_values` now always yields exactly one string +per `image_url`-typed part — `""` when nothing resolves — instead of +skipping. `""` fails `startswith("data:")` (closes the SSRF gap) and still +counts toward `any()`/`sum()` (closes the detection gap), matching what the +four original implementations did before this PR touched them. + +Two regression tests added: +- `tests/test_capabilities.py::test_a_degenerate_image_url_part_still_counts` +- `tests/test_chat_completions.py::test_local_fallback_refuses_a_degenerate_image_url_part` + +Full suite: 418 passed. + +## Take for next time + +The pattern worth naming: collapsing N independent implementations into one +shared primitive is exactly where behavior quietly narrows, because each +original implementation's handling of its own edge cases has to be +re-derived rather than copied — nothing forces the unifier to enumerate +what every caller did on the input shapes it doesn't have a test for. Plan +item #6 anticipated the collapse but not this; a shared traversal helper is +a good candidate for the plan itself to have specified "what does this +yield when a part is well-typed but unparseable" up front, rather than +leaving it to be inferred from four call sites with four different +fallback behaviors. diff --git a/config.py b/config.py index a7c09ce..3979190 100644 --- a/config.py +++ b/config.py @@ -216,7 +216,7 @@ class VerificationConfig(StrictModel): class LocalVisionConfig(StrictModel): - enabled: bool = False + enabled: bool = True base_url: str = "http://localhost:11434/v1" # OpenAI-compatible (classifier shape) api_key_env: Optional[str] = None model: str = "qwen3-vl:4b" diff --git a/dispatcher.py b/dispatcher.py index 4fd0fb1..b870121 100644 --- a/dispatcher.py +++ b/dispatcher.py @@ -53,11 +53,12 @@ from openai import OpenAI, OpenAIError from pydantic import BaseModel, Field import logs -from capabilities import detect_capabilities +from capabilities import detect_capabilities, iter_image_url_values from config import RouterConfig, load_config from routing import ( BATCH, INTERACTIVE, + capability_gate_reason, rank_candidates, rejection_reason, select_candidates, @@ -1410,15 +1411,7 @@ def _count_images(messages: list[dict]) -> int: ``detect_capabilities``. A part counts only when it is a dict whose type is ``image_url``, mirroring the detection rule. """ - count = 0 - for m in messages: - content = m.get("content") - if not isinstance(content, list): - continue - for part in content: - if isinstance(part, dict) and part.get("type") == "image_url": - count += 1 - return count + return sum(1 for _ in iter_image_url_values(messages)) def _image_payload_bytes(messages: list[dict]) -> int: @@ -1430,21 +1423,7 @@ def _image_payload_bytes(messages: list[dict]) -> int: image_url value so an oversized upload is skipped rather than POSTed to a local model that would reject it. """ - total = 0 - for m in messages: - content = m.get("content") - if not isinstance(content, list): - continue - for part in content: - if not isinstance(part, dict) or part.get("type") != "image_url": - continue - parts = part.get("image_url") - if isinstance(parts, dict): - # Inline base64 lives in `url` as data:image/...;base64,. - total += len(parts.get("url", "")) - elif isinstance(parts, str): - total += len(parts) - return total + return sum(len(v) for v in iter_image_url_values(messages)) def _local_vision_data_uris_ok(messages: list[dict]) -> bool: @@ -1460,19 +1439,7 @@ def _local_vision_data_uris_ok(messages: list[dict]) -> bool: URL declines the fallback (falls through to the 422) rather than handing a network-fetching capability to an unauthenticated caller. """ - for m in messages: - content = m.get("content") - if not isinstance(content, list): - continue - for part in content: - if not isinstance(part, dict) or part.get("type") != "image_url": - continue - value = part.get("image_url") - if isinstance(value, dict): - value = value.get("url", "") - if not isinstance(value, str) or not value.startswith("data:"): - return False - return True + return all(v.startswith("data:") for v in iter_image_url_values(messages)) def _run_local_vision(messages: list[dict], lv_cfg) -> Optional[str]: @@ -1493,16 +1460,18 @@ def _run_local_vision(messages: list[dict], lv_cfg) -> Optional[str]: if not _local_vision_data_uris_ok(messages): logs.warning("local_vision_skip", reason="remote_url") return None - if _count_images(messages) > lv_cfg.max_images: + n_images = _count_images(messages) + if n_images > lv_cfg.max_images: logs.warning( "local_vision_skip", reason="too_many_images", - n=_count_images(messages), max=lv_cfg.max_images, + n=n_images, max=lv_cfg.max_images, ) return None - if _image_payload_bytes(messages) > lv_cfg.max_image_bytes: + payload_bytes = _image_payload_bytes(messages) + if payload_bytes > lv_cfg.max_image_bytes: logs.warning( "local_vision_skip", reason="oversized_image", - bytes=_image_payload_bytes(messages), max=lv_cfg.max_image_bytes, + bytes=payload_bytes, max=lv_cfg.max_image_bytes, ) return None url = f"{lv_cfg.base_url.rstrip('/')}/chat/completions" @@ -1600,13 +1569,32 @@ def _local_vision_response(content: str, *, streaming: bool) -> Response: return StreamingResponse(stream(), media_type="text/event-stream") +def _model_exists(model_id: str) -> bool: + """Whether a bare id names a real routable catalog row. + + Used to decide whether a `provider/model` string a client sent is the + client's own provider alias (opencode's `llm-router/...` convention, + never a real NeuralWatt id) and should be stripped, or a real id that + happens to contain a slash -- which would not resolve once stripped. + """ + conn = _db() + try: + row = conn.execute( + "SELECT 1 FROM models WHERE model_id = ? AND provider = 'neuralwatt'", + (model_id,), + ).fetchone() + finally: + conn.close() + return row is not None + + def _check_pinned_capabilities(model_id: str, caps) -> None: """Reject a pinned model that cannot satisfy the request's capabilities. Reads the ``models`` row by ``model_id`` and fails closed on any missing - or NULL capability flag, matching the routing gate: a capability that - cannot be confirmed is treated as absent, because dispatching to a model - that might lack it is a guaranteed provider 400. + or NULL capability flag, matching the routing gate. Honors the + ``cfg.routing.require_*`` config flags so a pin and routed traffic face + the same gate — see issue #2 in the capability-gate follow-ups doc. """ conn = _db() try: @@ -1617,15 +1605,17 @@ def _check_pinned_capabilities(model_id: str, caps) -> None: ).fetchone() finally: conn.close() - if caps.has_images and (row is None or row["supports_vision"] != 1): + reason = capability_gate_reason( + dict(row) if row else {}, + require_vision=cfg.routing.require_vision and caps.has_images, + require_json_mode=cfg.routing.require_json_mode and caps.require_json_mode, + ) + if reason is not None: + capability = "vision" if reason.startswith("vision") else "json mode" raise HTTPException( 422, - f"{model_id} does not support vision; the request carries image parts", - ) - if caps.require_json_mode and (row is None or row["supports_json_mode"] != 1): - raise HTTPException( - 422, - f"{model_id} does not support json mode; the response_format requires it", + f"{model_id} does not support {capability}; " + f"the request {'carries image parts' if 'vision' in reason else 'requires response_format'}", ) @@ -1703,12 +1693,20 @@ def chat_completions(body: dict[str, Any], background: BackgroundTasks): caps = detect_capabilities(body) requested = body.get("model") or ROUTER_MODEL - # Some clients (opencode among them) send the model as `provider/model`. - # Real ids can contain slashes too (`deepseek-ai/DeepSeek-V4-Flash`), so - # only the virtual names are matched against a stripped suffix. + # Some clients (opencode among them) send the model as `provider/model`, + # for a pin as much as for `auto`. Real ids can contain slashes too + # (`deepseek-ai/DeepSeek-V4-Flash`), so a stripped suffix is only trusted + # once it resolves to a real catalog row -- otherwise the id, slash and + # all, is what gets dispatched, on the chance the slash is really part of + # it. Without this, a pin sent as `llm-router/gemma-4-31b` kept its + # prefix past this point: the capability pre-check looked up a model_id + # the catalog has never heard of, found no row, and fail-closed a pin + # that could satisfy the request into a false 422 -- and even past that + # check, the same unstripped id would have gone upstream and drawn a 400 + # from NeuralWatt, which only knows the bare form. bare = requested.rsplit("/", 1)[-1] wants_routing = bare in (ROUTER_MODEL, ROUTER_MODEL_BATCH) - if wants_routing: + if wants_routing or (bare != requested and _model_exists(bare)): requested = bare logs.debug( @@ -1758,6 +1756,7 @@ def chat_completions(body: dict[str, Any], background: BackgroundTasks): if decision.selected is None: if ( caps.has_images + and not caps.require_json_mode and cfg.local_vision.enabled ): fallback = _run_local_vision(messages, cfg.local_vision) diff --git a/opencode.json b/opencode.json index ff215e1..91db3f1 100644 --- a/opencode.json +++ b/opencode.json @@ -44,8 +44,7 @@ }, "modalities": { "input": [ - "text", - "image" + "text" ] } }, diff --git a/routing.py b/routing.py index 02c3f40..958aa10 100644 --- a/routing.py +++ b/routing.py @@ -33,6 +33,32 @@ INTERACTIVE = "interactive" BATCH = "batch" +def capability_gate_reason( + row: dict, *, require_vision: bool = False, require_json_mode: bool = False, +) -> str | None: + """Same fail-closed rule ``rejection_reason`` uses for vision/json-mode, + pulled out so a pinned-model check can reuse it without re-implementing + it — see dispatcher._check_pinned_capabilities. + + Returns a reason string when the gate fails, or None when the row passes. + Reason order: vision(unknown), vision(unsupported), json_mode(unknown), + json_mode(unsupported). + """ + if require_vision: + vision = row.get("supports_vision") + if vision is None: + return "vision(unknown)" + if not vision: + return "vision(unsupported)" + if require_json_mode: + jm = row.get("supports_json_mode") + if jm is None: + return "json_mode(unknown)" + if not jm: + return "json_mode(unsupported)" + return None + + def rejection_reason( row: dict, *, @@ -107,18 +133,11 @@ def rejection_reason( # absent means "cannot confirm the capability", and routing a request that # needs vision or JSON mode to a model that might lack it is a guaranteed # provider 400. Same precedent as the context(unknown) gate above. - if require_vision: - vision = row.get("supports_vision") - if vision is None: - return "vision(unknown)" - if not vision: - return "vision(unsupported)" - if require_json_mode: - jm = row.get("supports_json_mode") - if jm is None: - return "json_mode(unknown)" - if not jm: - return "json_mode(unsupported)" + gate = capability_gate_reason( + row, require_vision=require_vision, require_json_mode=require_json_mode, + ) + if gate is not None: + return gate if min_tool_proficiency is not None: tool_score = row.get("tool_proficiency") diff --git a/tests/test_capabilities.py b/tests/test_capabilities.py index e1ce860..5e0beec 100644 --- a/tests/test_capabilities.py +++ b/tests/test_capabilities.py @@ -4,7 +4,7 @@ Each case proves one detection fires and that the others stay False, mirroring the "rejects and does not over-reject" shape of test_routing.py. """ -from capabilities import detect_capabilities +from capabilities import detect_capabilities, iter_image_url_values def test_no_parts_yields_no_capabilities(): @@ -25,6 +25,15 @@ def test_image_url_part_sets_has_images(): assert detect_capabilities(body).has_images is True +def test_a_degenerate_image_url_part_still_counts(): + # No `image_url` key and no bare `url` key -- spec-invalid, but a part + # that declares itself an image must still count as one rather than + # silently vanishing because its value can't be resolved. Skipping it + # would let a malformed part dodge the vision-capability gate. + body = {"messages": [{"content": [{"type": "image_url"}]}]} + assert detect_capabilities(body).has_images is True + + def test_image_in_an_earlier_message_counts(): body = { "messages": [ @@ -82,3 +91,78 @@ def test_reasoning_effort_sets_has_reasoning_request(): def test_reasoning_param_sets_has_reasoning_request(): assert detect_capabilities({"reasoning": {"enabled": True}}).has_reasoning_request is True + + +# --- iter_image_url_values contract -------------------------------------- +# +# Pins the shared helper's contract across the four input shapes of +# code_plans/shared-helper-unparseable-input-contract.md. The point of this +# test is the same as the contract: the well-typed-but-unparseable shape +# must YIELD ITS SENTINEL ("") rather than be skipped silently, because +# skipping drops the part out of the fail-closed image checks at once. + + +def test_iter_image_url_values_yields_valid_dict_url(): + # Common/valid shape: image_url as a dict carrying `url`. + messages = [{"content": [{"type": "image_url", "image_url": {"url": "https://x/img.png"}}]}] + assert list(iter_image_url_values(messages)) == ["https://x/img.png"] + + +def test_iter_image_url_values_alternate_valid_shapes(): + # Alternate valid shapes the four originals agreed on: + # image_url as a bare string, and a bare `url` key on the part itself. + messages = [ + {"content": [{"type": "image_url", "image_url": "https://x/bare.png"}]}, + {"content": [{"type": "image_url", "url": "https://x/barekey.png"}]}, + ] + assert list(iter_image_url_values(messages)) == [ + "https://x/bare.png", + "https://x/barekey.png", + ] + + +def test_iter_image_url_values_well_typed_unparseable_yields_sentinel(): + # Well-typed but unparseable: the part declares itself an image_url but + # carries neither `image_url` nor a bare `url` that resolves. It must + # still yield SOMETHING (the "" sentinel) rather than vanish, so a + # degenerate part cannot slip out of the fail-closed checks. This is the + # shape the plan exists to pin down. + messages = [{"content": [{"type": "image_url"}]}] + assert list(iter_image_url_values(messages)) == [""] + + +def test_iter_image_url_values_wrong_payload_still_yields_sentinel(): + # The `image_url` key is present but its value is not a dict or a string + # (an int, say). Presence of the key is enough to claim "this is an + # image"; the payload fails to resolve, so it yields the "" sentinel + # rather than being skipped -- the same fail-closed contract as a part + # with no key at all. + messages = [{"content": [{"type": "image_url", "image_url": 42}]}] + assert list(iter_image_url_values(messages)) == [""] + + +def test_iter_image_url_values_skips_wrong_typed_and_absent(): + # Wrong-typed / absent shapes fail the coarse check and are skipped; + # they contribute nothing and yield nothing. + messages = [ + "not a dict", + {"content": "plain string, not a list"}, + {"content": [{"type": "text", "text": "hello"}]}, + {"content": [{"type": "tool_use", "id": "t1"}]}, + ] + assert list(iter_image_url_values(messages)) == [] + + +def test_iter_image_url_values_mixed_shapes_preserve_order(): + # A degenerate part in the middle must still count (as "") and not + # disturb the ordering of the parts around it. + messages = [ + {"content": [{"type": "image_url", "url": "https://x/1.png"}]}, + {"content": [{"type": "image_url"}]}, + {"content": [{"type": "image_url", "image_url": {"url": "https://x/2.png"}}]}, + ] + assert list(iter_image_url_values(messages)) == [ + "https://x/1.png", + "", + "https://x/2.png", + ] diff --git a/tests/test_chat_completions.py b/tests/test_chat_completions.py index dd702df..7fd5e25 100644 --- a/tests/test_chat_completions.py +++ b/tests/test_chat_completions.py @@ -565,6 +565,34 @@ def test_no_vision_model_uses_local_fallback_when_enabled(router, monkeypatch): ), "the image_url part must survive into the local call" +def test_local_fallback_skips_when_json_mode_is_requested(router, monkeypatch): + """Local vision answers in prose; a json_object request must not use it. + + JSON mode -- not vision -- is what excluded every cloud candidate here, so + the fallback must not fire or it would silently break the json_object + contract with free-form text instead of the 422 naming the missing + capability. + """ + client, calls, db_path = router + _drop_cheap_by_tier(dispatcher, db_path) + monkeypatch.setattr(dispatcher.cfg.local_vision, "enabled", True) + _local_vision_fake(monkeypatch, calls, content="should not be used") + + resp = client.post( + "/v1/chat/completions", + json={ + "model": "auto", + "messages": _image_messages(), + "response_format": {"type": "json_object"}, + }, + ) + + assert resp.status_code == 422 + assert "json" in resp.json()["detail"] + local = [c for c in calls if c.get("local")] + assert not local, "the local vision fallback must not run for a json_object request" + + def test_local_fallback_respects_streaming(router, monkeypatch): client, calls, db_path = router _drop_cheap_by_tier(dispatcher, db_path) @@ -660,6 +688,59 @@ def test_a_pinned_non_vision_model_with_an_image_is_a_clear_422(router): assert not calls, "no provider call should happen for an impossible pin" +def test_a_pinned_non_vision_model_dispatches_when_vision_gate_is_off(router, monkeypatch): + """The pin check honors `routing.require_vision`, like routed traffic. + + `require_vision: false` accepts the occasional provider 400 in exchange + for not fail-closing on an unconfirmed flag; a pinned model must face the + same config switch as `auto` routing, not an unconditional 422. + """ + client, calls, _ = router + monkeypatch.setattr(dispatcher.cfg.routing, "require_vision", False) + + resp = client.post( + "/v1/chat/completions", + json={"model": DEAR, "messages": _image_messages()}, + ) + + assert resp.status_code == 200 + assert calls[0]["body"]["model"] == DEAR, "the pin must dispatch as asked" + + +def test_a_provider_prefixed_pin_is_resolved_for_the_capability_check(router): + """opencode sends a pin as `provider/model` too, not just for `auto`. + + Before the fix, `requested` kept its `llm-router/` prefix past this + point: the capability pre-check looked up a model_id the catalog has + never heard of, found no row, and fail-closed a genuinely capable pin + into a false 422. It must resolve to the bare id and go through, and the + provider must see the bare id too -- NeuralWatt has never heard of + `llm-router/...` either. + """ + client, calls, _ = router + resp = client.post( + "/v1/chat/completions", + json={"model": f"llm-router/{CHEAP}", "messages": _image_messages()}, + ) + + assert resp.status_code == 200 + assert calls[0]["body"]["model"] == CHEAP + assert resp.headers["X-Router-Model"] == CHEAP + + +def test_a_provider_prefixed_pin_still_fails_closed_when_incapable(router): + """The prefix-resolution fix must not turn into a bypass.""" + client, calls, _ = router + resp = client.post( + "/v1/chat/completions", + json={"model": f"llm-router/{DEAR}", "messages": _image_messages()}, + ) + + assert resp.status_code == 422 + assert "vision" in resp.json()["detail"] + assert not calls, "no provider call should happen for an impossible pin" + + def test_the_two_pass_reroute_passes_image_capability(router): """The measured-context reroute must carry the image gate, not lose it.""" client, calls, _ = router @@ -706,6 +787,36 @@ def test_local_fallback_refuses_a_remote_image_url(router, monkeypatch): assert not local, "a remote image URL must never reach the local vision endpoint" +def test_local_fallback_refuses_a_degenerate_image_url_part(router, monkeypatch): + """A part that declares itself an image but carries no readable value at + all must still fail the "every part is a verified data: URI" check -- + silently skipping it would let it slip past the SSRF guard the same way + a remote URL is refused above.""" + client, calls, db_path = router + _drop_cheap_by_tier(dispatcher, db_path) + monkeypatch.setattr(dispatcher.cfg.local_vision, "enabled", True) + _local_vision_fake(monkeypatch, calls, content="should not be used") + + messages = [ + { + "role": "user", + "content": [ + {"type": "text", "text": "describe this"}, + {"type": "image_url"}, + ], + } + ] + resp = client.post( + "/v1/chat/completions", + json={"model": "auto", "messages": messages}, + ) + + assert resp.status_code == 422 + assert "vision" in resp.json()["detail"] + local = [c for c in calls if c.get("local")] + assert not local, "an unverifiable image part must never reach the local vision endpoint" + + def test_local_fallback_wont_masquerade_an_empty_answer_as_200(router, monkeypatch): """A 200 with empty content is a failed answer, and must fall through to 422 rather than return an empty 200 — a hidden failure must not look like a win.""" diff --git a/tests/test_config_endpoints.py b/tests/test_config_endpoints.py index 0458b7b..312d73d 100644 --- a/tests/test_config_endpoints.py +++ b/tests/test_config_endpoints.py @@ -203,7 +203,7 @@ def test_the_shipped_config_loads_with_the_new_keys(raw): assert loaded.routing.require_vision is True assert loaded.routing.require_json_mode is True assert loaded.local_vision.model == "qwen3-vl:4b" - assert loaded.local_vision.enabled is False + assert loaded.local_vision.enabled is True def test_a_typo_in_require_vision_is_rejected(raw): @@ -222,7 +222,7 @@ def test_local_vision_can_be_disabled_from_config(raw): def test_local_vision_defaults_when_absent(raw): cfg = copy.deepcopy(raw) cfg.pop("local_vision") - assert RouterConfig(**cfg).local_vision.enabled is False + assert RouterConfig(**cfg).local_vision.enabled is True def test_nonpositive_local_timeout_is_rejected(raw): diff --git a/tests/test_routing.py b/tests/test_routing.py index 9773083..50bc443 100644 --- a/tests/test_routing.py +++ b/tests/test_routing.py @@ -7,6 +7,7 @@ proving it rejects and a case proving it does not over-reject. import pytest from routing import ( + capability_gate_reason, is_eligible, rank_candidates, rejection_reason, @@ -440,6 +441,55 @@ def test_reasons_are_single_tokens(): assert " " not in reason +# --- capability_gate_reason (the extracted flag rule) ----------------------- +# +# rejection_reason delegates its vision/json-mode arm to this function, and +# dispatcher._check_pinned_capabilities reuses it so a pinned-model check and +# routed traffic cannot drift apart. It is unit-tested directly because the +# pinned-model path has no other cheap way to exercise these branches without +# a full request round-trip. + +def test_capability_gate_passes_when_not_required(): + # With no capability required, a model that lacks both still passes. + assert capability_gate_reason(_row(supports_vision=0, supports_json_mode=0)) is None + + +def test_capability_gate_rejects_missing_vision_flag(): + assert capability_gate_reason( + _row(supports_vision=None), require_vision=True + ) == "vision(unknown)" + + +def test_capability_gate_rejects_vision_unsupported(): + assert capability_gate_reason( + _row(supports_vision=0), require_vision=True + ) == "vision(unsupported)" + + +def test_capability_gate_rejects_missing_json_mode_flag(): + assert capability_gate_reason( + _row(supports_json_mode=None), require_json_mode=True + ) == "json_mode(unknown)" + + +def test_capability_gate_rejects_json_mode_unsupported(): + assert capability_gate_reason( + _row(supports_json_mode=0), require_json_mode=True + ) == "json_mode(unsupported)" + + +def test_capability_gate_admits_a_row_that_supports_both(): + assert capability_gate_reason( + _row(supports_vision=1, supports_json_mode=1), + require_vision=True, require_json_mode=True, + ) is None + + +def test_capability_gate_unknown_reason_on_empty_row(): + assert capability_gate_reason({}, require_vision=True) == "vision(unknown)" + assert capability_gate_reason({}, require_json_mode=True) == "json_mode(unknown)" + + def test_is_eligible_still_agrees_with_the_reason(): """One copy of the rules, two views of it.""" for row in (_row(), _row(tier=1), _row(latency_class="flex")):