# 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 `await`s 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.