6krrt/code_reviews/capability-gate-followups.md

# 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.