7d3d961cb0
Reviewing PR #3's iter_image_url_values (which correctly collapsed four traversals into one, per the capability-gate-followups plan): a part typed image_url but carrying neither a nested image_url.url nor a bare url key now yields nothing at all instead of an empty string, so it vanished from every derived check instead of failing them. Confirmed live before this fix: has_images read False (dodging the vision gate) and _local_vision_data_uris_ok read True (an SSRF guard passing vacuously on zero yielded items) for exactly this shape -- both regressions from what the four original hand-rolled loops did. Now every image_url-typed part yields exactly one string, "" when nothing resolves, which fails startswith("data:") and still counts toward any()/sum() -- matching pre-PR behavior. Two regression tests added. Also renames bugs_to_fix/ to code_reviews/, since this is the second report that's gone in there and "bugs to fix" undersold what the first one turned into -- and adds a review report for PR #3 alongside the plan it was implementing.
4.8 KiB
Review: PR #3, "fix: resolve remaining capability-gate review findings"
What it was reviewing: the implementation of
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:
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 moderequire_visionfail-closed-on-unknown exists to prevent._local_vision_data_uris_ok→True: the SSRF guard (every image part must be a verifieddata:URI before the local model is trusted with it) passed vacuously, sinceall()over zero yielded items isTrue.
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_countstests/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.