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:

```python
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:

```python
>>> 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):

```python
>>> 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:

```python
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.