6krrt/plans/flex-preference-knob-review.md

flex-preference-knob review

Status: done -- review of shipped work

Source: manual review (this session) of 65da4a1 ("feat(routing): add flex-preference knob for -flex variant selection") against .omo/plans/flex-preference-knob.md. Full suite is green (601 passed) and the plan's stated behavior — swap only the serving class, never the base model, re-price on swap, log telemetry — checks out. One finding, confirmed live against routing.py and against the current catalog, not just read off the diff.

---

1. A flex swap bypasses every hard filter except latency

File: routing.py, apply_flex_preference (~line 195-269), called from dispatcher.route() (~line 798-811).

Problem. get_flex_sibling is deliberately structural — it matches a row by base_model_id/reasoning_mode/context_variant + latency_class == "flex" and nothing else. Its own test says why that's fine:

def test_flex_sibling_ignores_stale_or_unroutable_flex_rows():
    # The helper is purely structural: it matches by identity dims regardless
    # of freshness/access. Routing-level gating is the caller's job.

But the caller — apply_flex_preference — never does that gating. It checks exactly one thing before swapping: latency_tolerance == INTERACTIVE (for prefer-flex; force-flex doesn't even check that). It never re-runs rejection_reason/is_eligible against the sibling, so a swap can hand back a row that select_candidates would have thrown out for being stale, deprecated, outside allowed_access_levels, under-tiered, or missing a required capability. The comment justifying the latency-only check — "the only hard filter that distinguishes it is the latency filter" — is an assumption about the catalog, not something the code checks, and the live catalog already violates the premise: glm-5.2 (access_level: canary) and glm-5.2-flex (access_level: public) share base_model_id/ reasoning_mode/context_variant and differ in access level, not just latency class. Nothing stops that divergence from appearing in the other direction (a -flex row going stale, deprecated, or more restricted than its standard sibling) — the poller marks stale/deprecated per row from each row's own last_updated, and access level is parsed from the provider's free-text description per row (poller.parse_access_level), independently for every model_id.

Confirmed live, calling apply_flex_preference directly:

>>> from routing import apply_flex_preference
>>> std = {"model_id": "kimi-k3", "base_model_id": "kimi-k3",
...        "reasoning_mode": "default", "context_variant": "full",
...        "latency_class": "standard", "cost": 0.01,
...        "proficiency_score": 0.9, "cost_score": 0.5, "composite": 0.9,
...        "cost_per_1m_prompt": 1.0, "cost_per_1m_completion": 2.0}
>>> stale_flex = {**std, "model_id": "kimi-k3-flex", "latency_class": "flex",
...               "availability": "stale"}
>>> apply_flex_preference(std, [std, stale_flex], "force-flex", "interactive",
...     prompt_tokens=1000, completion_tokens=400, cache_rate=0.5)
({'model_id': 'kimi-k3-flex', ..., 'availability': 'stale'}, True, True, ...)

and the same for an access-restricted sibling under prefer-flex/batch (no force needed — prefer-flex doesn't check access either):

>>> canary_flex = {**std, "model_id": "glm-5.2-short-flex",
...                 "latency_class": "flex", "access_level": "canary"}
>>> apply_flex_preference(std, [std, canary_flex], "prefer-flex", "batch",
...     prompt_tokens=1000, completion_tokens=400, cache_rate=0.5)
({'model_id': 'glm-5.2-short-flex', ..., 'access_level': 'canary'}, True, False, ...)

Both swaps go through with no error, and dispatcher.route() puts the result straight into RouteResponse.selected — there is no gate between apply_flex_preference and the actual provider call. A restricted-access swap dispatches to a model the account almost certainly gets a 403 from; a stale/deprecated swap dispatches to a row the freshness filter exists specifically to keep traffic away from. This is exactly the "silent empty axis" failure class the project has hit before (the 6e729ad/1d1f3af seed-sweep crash, the client_capped over-marking) — a real check exists elsewhere in the codebase (rejection_reason) and this new path just doesn't call it.

Not just access/freshness: the same gap covers required_tier, effective_context_window, min_tool_proficiency, and the vision/json-mode capability gates, for the same reason — none of them are re-checked on the sibling. Access and staleness are the two proven-live cases; the others are plausible but unconfirmed against current data.

Fix. apply_flex_preference needs the same filter arguments select_candidates/rejection_reason already take, and must refuse the swap unless the sibling clears them. The cleanest way to get "every filter except latency" without a second copy of the rule is to call rejection_reason on the sibling with latency_tolerance forced to BATCH (which always admits flex rows), so only the non-latency filters can reject it:

def apply_flex_preference(
    selected_row, all_candidates, flex_preference, latency_tolerance,
    *, prompt_tokens, completion_tokens, cache_rate,
    required_context_tokens, required_tier, allowed_access_levels,
    exclude_stale, exclude_deprecated, min_tool_proficiency=None,
    require_vision=False, require_json_mode=False,
) -> tuple[dict, bool, bool, float | None]:
    ...
    sibling = get_flex_sibling(selected_row, all_candidates)
    if sibling is None:
        return selected_row, False, False, selected_row.get("cost")

    # The sibling is a different catalog row and can independently be stale,
    # deprecated, access-restricted, or under-tiered -- BATCH is used here
    # only to neutralize the latency filter itself, which prefer-flex/
    # force-flex evaluate separately below.
    if rejection_reason(
        sibling,
        required_context_tokens=required_context_tokens,
        required_tier=required_tier,
        latency_tolerance=BATCH,
        allowed_access_levels=allowed_access_levels,
        exclude_stale=exclude_stale,
        exclude_deprecated=exclude_deprecated,
        min_tool_proficiency=min_tool_proficiency,
        require_vision=require_vision,
        require_json_mode=require_json_mode,
    ) is not None:
        return selected_row, False, False, selected_row.get("cost")
    ...

dispatcher.route() already builds exactly this filter set as filters (line ~749-762, used for select_candidates) — pass it through to apply_flex_preference rather than assembling a second, narrower one.

Test to add (tests/test_routing.py, next to the existing test_apply_force_flex_* cases): a flex sibling with availability="stale" and one with access_level="canary" (allowed levels ["public"]), under both prefer-flex/batch and force-flex/interactive — assert no swap (flex_swapped is False, original selected_row returned) in all four combinations. tests/test_route_decisions.py's decision_router fixture only ever inserts CHEAP_FLEX as public/active; add a second flex fixture row with availability="stale" for an end-to-end version of the same check through /route.

---

Everything else checked out

• get_flex_sibling is pure, O(N), matches the plan's three identity dimensions exactly, and correctly returns None when the selected row is already flex.
• route_decisions migration (ensure_route_decisions) is a proper idempotent ALTER TABLE ... ADD COLUMN guarded by PRAGMA table_info, matching the proficiency_store.ensure_columns pattern this project already uses elsewhere.
• flex_preference/flex_swapped/flex_forced telemetry is wired correctly end-to-end: RouteResponse → persist_route_decision → route_decisions INSERT → SSE event payload. This was the one thing flagged as worth re-checking when the config-default question was settled (project-flex-preference-knob), and it holds up — confirmed by reading the full call chain, not just the happy-path tests.
• Cost re-estimation on swap reads the sibling's own cost_per_1m_prompt/cost_per_1m_completion via estimated_cost, and carries over proficiency_score/cost_score/composite from the ranked winner so the returned dict stays a well-formed Candidate — correct, since a flex twin ties its standard sibling on every scored dimension by construction.
• no-flex/auto are true no-ops (never swap, never touch an already-flex winner for no-flex), matching the plan.
• Full pytest (601 tests) and python config.py both pass clean.