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
4.8 KiB
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, 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.