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