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.
85 lines
4.8 KiB
Markdown
85 lines
4.8 KiB
Markdown
# 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.
|