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
119 lines
5.9 KiB
Markdown
119 lines
5.9 KiB
Markdown
# Review: `admin-visual-fixes` implementation
|
|
|
|
Status: done -- review of shipped work
|
|
|
|
**What it was reviewing:** the 8-todo `admin-visual-fixes` plan
|
|
(`plans/.omo/plans/admin-visual-fixes.md`), after its orchestration
|
|
reported 8/8 complete and F1-F4 all APPROVE. Verified against the actual
|
|
diff and the plan's own saved QA evidence
|
|
(`plans/.omo/evidence/task-8-admin-visual-fixes-console.txt`), not the
|
|
orchestration summary.
|
|
|
|
## Verdict: 6 of 8 todos are genuinely fixed; Todo 2 is not, and the plan's own evidence file proves it
|
|
|
|
### Confirmed correct
|
|
- Todo 1 (quota bar): `gauge-canvas` fully removed, `.quota-bar` markup in place.
|
|
- Todo 3 (history chart): canvas id fixed to `history-chart` **and**
|
|
`historyChart = renderChart(...)` correctly captures the instance
|
|
(`admin/frontend/index.html:610`).
|
|
- Todo 4: `knobDisplayValue()` (`index.html:687-696`) correctly unwraps
|
|
`{enabled: bool}` before display.
|
|
- Todo 5: `renderWarnings` (`index.html:538-547`) rebuilds the `#no-warnings`
|
|
placeholder inline instead of referencing a detached node — no more stale
|
|
reference.
|
|
- Todo 6 (XSS): `task_category` and `modelStr` (selected_model/
|
|
selected_provider) are now `escapeHtml()`-wrapped in both
|
|
`renderDecisions` and `addDecision`. `rejected_reason`/`runner_up_models`
|
|
are never rendered anywhere in the file, so there was nothing to escape
|
|
there.
|
|
- Todo 7 (config race): correct and thorough.
|
|
`_persist_config_value` (`admin.py:349-374`) takes a module-level
|
|
`threading.Lock` around the whole load→validate→backup→write→replace
|
|
sequence (the right primitive — `admin_config_write` is a *sync* handler,
|
|
which FastAPI runs via `anyio.to_thread`, so `asyncio.Lock` would not have
|
|
serialized anything), validates before backing up, and writes via
|
|
`.tmp` + `os.replace` so a reader never observes a truncated file.
|
|
`tests/test_admin_config.py::test_config_concurrent_writes_are_atomic_no_zero_byte_backups`
|
|
is a real regression test — a live reader thread asserts the file is
|
|
never empty while two writer threads race. `saveAllConfig()`
|
|
(`index.html:774-804`) is now a sequential `for...of await` loop, not
|
|
`Promise.all`. All 57 admin tests pass.
|
|
|
|
### Not fixed: Todo 2 (verdict doughnut) — and the evidence already proves it
|
|
|
|
`renderVerdict` (`index.html:508-536`) fixed the canvas id
|
|
(`verdict-canvas` → `verdict-chart`) but never assigns the created chart
|
|
back to the `verdictChart` variable:
|
|
|
|
```js
|
|
if (verdictChart) { updateChart('verdict', labels, values, 'doughnut', colors); }
|
|
else { renderChart('verdict-chart', labels, values, 'doughnut', colors); } // return value dropped
|
|
```
|
|
|
|
`verdictChart` stays `null` forever, so every 30-second poll
|
|
(`REFRESH_MS`) takes the `else` branch again and tries to create a second
|
|
Chart.js instance on a canvas that already has one attached. This is
|
|
already recorded in the plan's own saved evidence,
|
|
`task-8-admin-visual-fixes-console.txt`:
|
|
|
|
```
|
|
Uncaught (in promise) Error: Canvas is already in use. Chart with ID '1' must be
|
|
destroyed before the canvas with ID 'verdict-chart' can be reused.
|
|
at renderChart (http://127.0.0.1:8091/admin/:831:11)
|
|
at renderVerdict (http://127.0.0.1:8091/admin/:531:9)
|
|
at loadSnapshot (http://127.0.0.1:8091/admin/:332:4)
|
|
```
|
|
|
|
— repeated 6 times in the log. The doughnut renders once on first load,
|
|
then throws and stops updating for the rest of the page's life. This
|
|
directly contradicts F3's "clean console" sign-off and Todo 2's own
|
|
acceptance criteria ("the verdict doughnut renders"); a single screenshot
|
|
taken right after page load wouldn't catch it, but the saved console log
|
|
already had it and nobody read it before approving.
|
|
|
|
**Fix**: mirror what `historyChart` already does correctly two functions
|
|
over — capture the return value:
|
|
|
|
```js
|
|
else { verdictChart = renderChart('verdict-chart', labels, values, 'doughnut', colors); }
|
|
```
|
|
|
|
Also update the dead-code branch at `index.html:512`
|
|
(`renderChart('verdict-chart', null, 'verdict')` — missing arguments,
|
|
never actually reassigns anything either) to just `verdictChart = null;`
|
|
without the pointless re-render call, since there's no data to draw.
|
|
|
|
### Secondary, out-of-scope-but-worth-a-look: `objective.max_energy_per_request` 422s on every "Save All"
|
|
|
|
Same evidence file shows two `422` responses for
|
|
`POST /admin/api/config/objective.max_energy_per_request` during the
|
|
save-all QA pass. `config.yaml` has this key as `null`; the config table's
|
|
text input renders it as an empty string, and `saveAllConfig`'s
|
|
number-coercion (`Number('') → 0`, but the `val.trim() !== ''` guard
|
|
correctly keeps it as `''` rather than coercing to `0`) ends up POSTing an
|
|
empty string against an `Optional[float]` field, which `RouterConfig`
|
|
rejects. Not data-destructive (validation fails before the backup/write
|
|
step, exactly as designed) and not part of any of the 8 todos' scope, but
|
|
worth a follow-up: null-valued numeric config fields can't currently be
|
|
round-tripped through "Save All" without a spurious failure toast.
|
|
|
|
### Process gap, not a code defect
|
|
|
|
No commits landed for any of the 8 todos despite the plan's explicit "one
|
|
commit per todo (8 total)" commit strategy — `git log` still ends at
|
|
`b63234a`, and `admin.py` / `admin/frontend/index.html` remain untracked.
|
|
Separately, the live `llm-router.service` has not restarted since 02:35:45
|
|
(same PID throughout), so the Todo 7 fix — the one that matters for
|
|
production — is sitting on disk but not yet protecting the running
|
|
service.
|
|
|
|
## Recommendation
|
|
|
|
Fix the one-line `verdictChart` assignment (plus the dead branch at
|
|
`:512`), re-run the same Playwright QA against a fresh page load *and* at
|
|
least one 30s+ idle period to confirm the console actually stays clean
|
|
across a poll cycle this time — a single screenshot won't catch this class
|
|
of bug again. Then commit per the plan's own commit strategy. Restarting
|
|
`llm-router.service` to pick up the Todo 7 fix is a separate, explicit step
|
|
to take afterward, not bundled into the same action.
|