6krrt/plans/router-admin-portal-plan-review.md

Review: router-admin-portal work plan (pre-implementation)

Status: done -- review of shipped work

What it was reviewing: plans/.omo/plans/router-admin-portal.md — a 10-todo plan to add an integrated /admin FastAPI web portal (dashboards

• operator controls: refresh catalog, reseed energy, restart service, runtime toggles, allowlisted config writes, model-availability overrides). Reviewed before any implementation exists, against the actual current code rather than the plan's own descriptions, and cross-checked against plans/admin-portal-gap-analysis.md (a 15-finding critique of an earlier draft, plans/.omo/drafts/router-admin-portal.md) to see whether the final plan actually incorporated what that analysis found.

Verdict: citations are excellent; one likely functional gap and one unexamined security assumption before this should be approved as-is

Citation accuracy — verified, essentially flawless

Checked every file:line reference that matters for correctness, not a sample:

• dispatcher.py:154 (cfg), :278 (_db), :1306/:1353/:1417 (/health//metrics//events/decisions) — all exact.
• config.py:185 (default_flex_preference: FlexPreference = FlexPreference.auto) — confirmed real field, and every one of its four cited use sites (dispatcher.py:815, 2364, 2446 plus the docstring mention at :186) — exact.
• Todo 5's entire runtime-toggle line list — eleven citations across seven config knobs (circuit_breaker.enabled at 2556/2573/2820/2823, log_route_decisions at 935, log_energy_observations at 1261, verification.local_llm_enabled at 2653/2794, session_cache.enabled at 2266/2336, pinch.enabled at 2247/2475, the pinch.relevance gate at 1827) — every single one checked out exact via direct grep.

One trivial slip: Todo 3 cites dispatcher.py:2126-2125 for /v1/models — a backwards range (start line > end line); the actual decorator is a single line at 2126. Cosmetic, not worth blocking on.

The admin override table may not actually affect routing — verify before approving

Todo 7 creates admin_model_overrides(model_id, provider, availability, reason, updated_at) specifically to survive poller.py overwriting models.availability on every catalog refresh. That part is correctly diagnosed: poller.py's upsert really does set availability = "active" on every routable row it sees, per the gap analysis's F1 (confirmed independently, not taken on the gap analysis's word).

But Todo 7's own text only says GET /admin/api/models merges the override into what it returns to the dashboard. Nowhere across Todos 1–10 is there a step that feeds admin_model_overrides into routing.select_candidates / routing.rejection_reason — the functions that actually decide what a live request routes to. As specified, a model marked "deprecated" through the admin UI would show as deprecated on the dashboard while the router keeps dispatching to it exactly as before, because the hard filters still only read models.availability / models.deprecated, which the override table never touches.

This is the same shape of defect the last review caught in circuit_breaker.record_success: a feature that is structurally complete — table, API, tests — but never reaches the code path it exists to affect. It matters more here, because the stated purpose of this whole control ("mark a flaky model out of rotation") is exactly what was done by hand with a raw UPDATE models SET deprecated = 1 ... earlier this session when gemma-4-31b started failing — if the admin UI's version of that same action doesn't reach routing.py, the feature doesn't do the thing it's being built for. Add an explicit todo (or fold into Todo 7's acceptance criteria) requiring route()'s candidate-filtering to consult the override table the same way _open_circuits already consults circuit_breaker.is_down for the circuit-breaker exclusion set, and add a test that actually routes a request against an overridden model and asserts it's excluded — not just that the API reports it correctly.

Loopback-only/no-auth is inherited from a read-only posture without being re-examined for a read-write one

The plan states "loopback-only and unauthenticated, matching the router's current security posture" as an already-made decision. The current posture (/metrics, /health, SSE decisions) is read-only observability, and loopback-only is a reasonable ACL for that. This plan adds POST /admin/api/restart-service, on-disk config writes, and subprocess triggers — a materially larger blast radius reachable by the same unauthenticated loopback bind. Loopback-only does not mean "only the operator can reach it": with no auth and no CSRF protection specified anywhere in the plan, any process running as the same user, or any webpage the operator's browser visits while the router happens to be running, can POST to localhost:8080/admin/api/restart-service via a plain HTML form — no preflight required to block a same-origin-policy-naive form POST. This is a known category of local-dev-server attack (CSRF/DNS-rebinding against localhost) that a pure read-only surface never had to consider. Worth a deliberate decision — even if the answer stays "loopback-only is acceptable for a personal single-operator tool" — rather than carrying the old answer forward because it was already the answer for a different kind of surface.

Separately, Todo 4's restart-service mechanism is stated aspirationally ("is expected to terminate the serving process; return a restarting status before the process exits") without specifying how the HTTP response gets flushed to the client before systemctl --user restart causes SIGTERM to land on the same process handling that request. If the endpoint awaits the systemctl subprocess inline, the response can't be sent before the process that would send it is torn down. This needs an explicit fire-and-forget mechanism (e.g., schedule the restart via BackgroundTasks so it runs after the response is already flushed) named in the plan, not left as an implementation detail to be discovered while writing Todo 4.

Three "Recommended" gap-analysis findings are silently absent, not declared as deferred

F9 (no audit trail for admin actions), F10 (no lock against concurrent config.yaml writes), and F11 (no endpoint to clear circuit-breaker/session cache state) are all "Recommended" rather than "Must-fix: Yes" in the gap analysis's own severity table, and none appear in the final plan's Must-have list. That's a defensible call for a personal, single-operator tool — but none of the three are named in the plan's "Must NOT have" section either, which is where deliberate exclusions are supposed to live. As written, a reader of the final plan alone has no way to tell "considered and deferred" from "the gap analysis was never actually read." Move them there explicitly, even as a one-line "deferred: F9/F10/F11, personal-use tool, revisit if this gets multi-operator" note.

Cross-repo citations also check out — including the parts I could actually verify

daashbrd exists locally (/home/alee/Sources/daashbrd), so the plan's references to it aren't unverifiable hand-waving. Checked directly: app/history.py and app/frontend/index.html (2303 lines, matching Todo 12's "fine for a single-page <3k line file" claim in the gap analysis) exist as cited; app/main.py:154 is exactly the StaticFiles.mount(...) call and :303 is exactly the FileResponse(...) call the gap analysis's F12 cited; tests/test_main.py:23 and :29 are exactly test_frontend_dir_is_absolute_and_exists and test_static_mount_points_to_frontend_dir, matching "mount checks." poller.py's main()/__main__ at 298/319, feedback.py at 143/171, tier.py at 82/92, and schema.sql's models table (line 7) and its three indexes (244-246) all match Todo 4's and Todo 7's citations exactly as well. At this point essentially every checkable citation in the entire plan has been verified — this is the most citation-accurate plan reviewed in this loop so far.

Todo 2's history store duplicates data the router already durably records

admin_history.py is specified as a separate SQLite file (router_admin_history.db) fed by a background task sampling every 60s — a pattern lifted directly from daashbrd, which doesn't have anything better to query. This router already isn't in that position: energy_observations records energy_kwh, cost_usd, and carbon_g_co2eq per request, as it happens, and route_decisions records one row per routing decision — both timestamped, both already durable. metrics.py's existing /metrics endpoint already computes rolling aggregates from energy_observations on demand, no sampling loop required.

A bucketed time-series GET /admin/api/history can almost certainly be a GROUP BY-bucketed SQL query against the tables that already exist, computed on request rather than sampled every 60s into a second database. That would avoid three problems the current spec creates and doesn't solve: (1) a background-task lifecycle question that Todo 2 doesn't actually answer — admin.py must never import dispatcher per Todo 1, so nothing in the plan specifies what schedules this recurring task against the running event loop or on which object's startup hook; (2) a second SQLite file whose freshness is bounded by a 60s sampling interval instead of being exact; (3) genuinely duplicated data. Given this router's real traffic volume (one personal deployment; config.yaml's own comments mention total billed traffic to date as $0.07), query performance against the existing tables is not a concern that justifies pre-aggregation. Worth reconsidering before Wave 1, since it's foundational to Todo 2's whole shape.

ruamel.yaml is a new dependency with no todo to pin it

Todo 6 commits to "ruamel.yaml round-trip preservation" for config writes, but ruamel.yaml is not in requirements.txt today (checked directly — zero matches) and no todo in the plan adds it. CLAUDE.md's own README section is explicit and deliberate about this: dependencies are pinned specifically so "a service that restarts on boot shouldn't change its dependency tree underneath itself," and bumps are meant to be deliberate. Introducing a new library for a real reason (comment-preserving YAML writes is the right tool for what F2 needed) is a legitimate, deliberate addition — but it needs its own explicit line in requirements.txt with a pinned version, and a todo (or an amendment to Todo 6) that says so, rather than being assumed available.

Minor: "single self-contained HTML file" and a /admin/static mount say two different things

Todo 8 describes "a single self-contained HTML file using Chart.js from CDN" (matching gap-analysis F12's option (a): one HTMLResponse string, no static mount needed). Todo 9 then separately mounts admin/frontend/ under /admin/static. If the page is genuinely self-contained with only a CDN script tag, there's nothing left to serve under a static mount and it can be dropped; if there are separate static assets planned (a favicon, a stylesheet), "self-contained" should say so. Small inconsistency, easy to resolve either direction — flagging so whichever way it goes is a decision rather than a leftover from copying daashbrd's shape wholesale.

Recommendation

Don't approve as-is. Four things need an answer before implementation starts, not after — none of them require redoing work already done well:

1. Confirm — or add a todo requiring — that admin_model_overrides actually reaches routing.select_candidates, since without that the model-management feature doesn't do what it's for.
2. Make an explicit, stated decision about auth/CSRF for the new write-capable endpoints rather than inheriting the read-only surface's answer unexamined, and specify the restart-service response-before-death mechanism concretely.
3. Reconsider Todo 2's separate sampled history database against just querying energy_observations/route_decisions directly — it's simpler, exact instead of 60s-stale, and doesn't leave the background- task lifecycle question unanswered.
4. Add the missing requirements.txt pin for ruamel.yaml (or a todo that does), matching this project's own stated policy on deliberate, pinned dependency changes.

The citation work underneath all of this is excellent — genuinely the most accurate plan reviewed in this loop so far, including cross-repo references into daashbrd that all checked out — and doesn't need rework. This is a plan worth sending back for one more pass on the four points above, not a rewrite.