6krrt/plans/session-cache-and-baseline-comparator-review.md

session-cache-and-baseline-comparator review

Status: done -- review of shipped work

Source: manual review of the batch built from session-classification-cache-ttl.md, baseline-routing-comparator.md, and classifier-input-scope-check.md. Full suite verified green independently (638 passed). One component is clean and committed; the other has a bug confirmed live against router.db, not just read off the diff — and is not actually committed yet, despite the delivery summary saying "all deliverables are complete and merged."

---

Session classification cache — clean, matches spec

Commits 245842d, ba5b50d, f4557d4, 0a26f58, 756341d. No findings.

Specifically verified:

• session_cache.py is pure (no I/O, no imports of dispatcher/config), TTL comparison is > not >= (a hit exactly on the boundary is still fresh, matching the spec's "exactly 60s later... not strictly greater... so fresh" framing), and get never extends an entry's life — only put writes a fresh timestamp.
• dispatcher.py's integration correctly reorders to compute measured tokens before the cache check, issues exactly one route() call on a cache hit (never a wasted classify-then-reroute), and captures classified_src before the override reroute can overwrite decision.classification.source — so the cache-write gate (classified_src == "classifier") reflects the original classification attempt, not whatever the source ends up as after a measured-context reroute.
• Fallback classifications are never written to the cache (tested directly: test_fallback_classification_is_never_cached).
• Capability flags stay fresh on a cache hit — tested directly (test_capability_flags_are_still_read_fresh_on_a_cache_hit) by sending a same-session image turn after a cached text turn and confirming it still routes to a vision-capable model and logs source="cached" for the routing decision while still applying the freshly-read has_images gate. This was the one property that mattered most in the spec, and it's the one with a dedicated test.

Baseline routing comparator — not committed, and wrong against real data

baseline_report.py and tests/test_baseline_report.py are untracked in git (git status --short shows both as ??), unlike every other file in this batch. Nothing here is merged.

1. load_decisions filters kind = 'route', which excludes 99.5% of real traffic

File: baseline_report.py:85 (load_decisions), consumed by analyze.

Problem. The query hardcodes kind = 'route'. Live router.db:

sqlite> SELECT kind, COUNT(*) FROM route_decisions GROUP BY kind;
chat|2209
route|10

"route" is the low-volume /route probe endpoint (no quota spent, mostly used for testing). "chat" is /v1/chat/completions — the actual dogfooding traffic this whole tool exists to audit, and it's 99.5% of the table. Running the report against the real database does not error, it just silently analyzes almost nothing:

$ python baseline_report.py --since 2026-08-01
baseline routing comparator
  decisions: 10
  ...

10 decisions, all of which are leftover manual test calls from this session's own debugging, not the dogfooding traffic the tool is meant to audit. This is exactly the failure mode the README repeatedly warns about in its own history — "scored the rig, not the model" — reproduced in a new tool: the test fixture (tests/test_baseline_report.py:96, INSERT INTO route_decisions (... kind ...) VALUES (?, 'route', ...)) only ever seeds kind='route' rows, so the suite is internally consistent and green without ever exercising what the live table actually contains. 638 passing tests didn't catch this because no test's fixture data resembled production shape here.

Fix. persist_route_decision is called with four kinds that represent an actual scored routing decision — "route" (dispatcher.py:1628), "chat" (:2255, :2278), and "dispatch" (:2664) — versus two that don't: "passthrough" (:2297, a client pin that bypasses scoring entirely) and "local_vision" (:2214, logged specifically because routing found no eligible cloud candidate — there's no "eligible set" to rank a baseline within). The filter should be kind IN ('route', 'chat', 'dispatch'), not kind = 'route'.

2. Rejected decisions (no selected_model) would pollute the aggregate once #1 is fixed

File: baseline_report.py:230-253 (analyze's accumulation loop).

Problem. dispatcher.py:2255 logs a "chat" row with rejected_reason set and no selected_model when routing finds nothing eligible (a 422 to the client — no work was actually dispatched). Once finding #1's filter is widened to include "chat", these rows enter analyze's loop. agg["actual_cost"] += d["est_cost_usd"] or 0.0 and agg["actual_proficiency"] += ap if ap is not None else 0.0 both silently add 0.0 for a request that was never served, which pools "the router refused this" together with "the router served this for free" in the actual_* aggregates — understating real average cost/proficiency for any category with rejections. cheapest["model_id"] == d["selected_model"] degrades gracefully here (None never matches, so it just doesn't count as dominance) but the cost/proficiency pollution does not degrade gracefully.

Not currently visible in router.db — SELECT COUNT(*) FROM route_decisions WHERE kind IN ('route','chat','dispatch') AND selected_model IS NULL returns 0 on this deployment right now — but it's a latent bug in the aggregation logic, not something that depends on luck staying the current way, and the fix is one clause.

Fix. Add selected_model IS NOT NULL to load_decisions's WHERE clause alongside the kind fix, so a rejected/unserved request is excluded from both the aggregate and the dominance count, the same way this project already excludes non-model-attributable failures elsewhere (model_attributable = 0 in the verification-feedback path).

Everything else in the file checks out

reconstruct_decision's filter-dict construction correctly gates min_tool_proficiency/require_vision/require_json_mode only when the original decision's tools/images/json_mode flags were set (mirroring capabilities.py's "only applies when the request carries it" rule), the cheapest/best-proficiency tie-breaks are deterministic (model_id as the final tie-break in both), and --csv/human-table output both read from the same analyze() result so they can't drift from each other. The "current catalog, not historical snapshot" limitation is stated plainly in the module docstring, matching the plan.

Recommendation

Fix both findings in baseline_report.py (one WHERE clause each), add a chat-kind fixture row to tests/test_baseline_report.py so the test suite would have caught #1, then commit. Session cache needs nothing further.