3523dcf93e
plans/ held 58 documents and exactly one said whether it was open. The rest
mixed finished work, reviews of shipped work, parked specs and genuinely
pending ones, with nothing distinguishing them, so "how many plans are in
the queue" had no answer short of reading all 58.
Now `grep -H '^Status:' plans/*.md` is the answer:
50 done 3 in progress 2 planned 2 reference 1 parked
Statuses were derived rather than guessed: CLAUDE.md's own built list and
"What's NOT built yet" section, plus checking the subject exists in the
code. A review of work that shipped counts as done -- it records what was
found, it is not a request for anything. `reference` separates the two docs
that are conventions rather than work items (admin-design-standards,
admin-work-framework), which otherwise read as permanently-open plans.
The vocabulary is deliberately five words. A larger one invites "mostly
done" and "blocked-ish", which is how the directory became unreadable.
test_plans_declare_status.py keeps it from rotting: a new plan without a
marker fails, as does an unknown status, one buried below the eighth line,
or an open status with no reason -- "planned" alone is the state that rots,
since nobody can tell later whether it waits on a decision, a dependency,
or just nobody's turn.
Also updates the sweep plan with what landed and what did not, including
that #9 was not a defect.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VRQXz5SYZYVWscxS1QqF6U
87 lines
4.8 KiB
Markdown
87 lines
4.8 KiB
Markdown
# Review: PR #3, "fix: resolve remaining capability-gate review findings"
|
|
|
|
Status: done -- review of shipped work
|
|
|
|
**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.
|