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
143 lines
6.9 KiB
Markdown
143 lines
6.9 KiB
Markdown
# 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`](session-classification-cache-ttl.md),
|
|
[`baseline-routing-comparator.md`](baseline-routing-comparator.md),
|
|
and [`classifier-input-scope-check.md`](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.
|