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
218 lines
12 KiB
Markdown
218 lines
12 KiB
Markdown
# 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.
|