From ae1fcced51bda09ee84335ca25eb9377cc4d0af8 Mon Sep 17 00:00:00 2001 From: adlee-was-taken Date: Fri, 25 Sep 2026 19:57:08 -0400 Subject: [PATCH 1/6] fix(admin): classifier card renders a loading state until config loads --- admin/frontend/controls.html | 22 +++++++-- tests/test_admin_frontend.py | 89 ++++++++++++++++++++++++++++++++++++ 2 files changed, 108 insertions(+), 3 deletions(-) diff --git a/admin/frontend/controls.html b/admin/frontend/controls.html index 51108f8..0fc9c97 100644 --- a/admin/frontend/controls.html +++ b/admin/frontend/controls.html @@ -261,7 +261,9 @@ header.navbar{padding-top:2px!important;padding-bottom:2px!important}
- + @@ -279,7 +281,7 @@ header.navbar{padding-top:2px!important;padding-bottom:2px!important}
- +
@@ -1111,9 +1113,23 @@ function renderClassifierCandidateCategories(data) { async function loadClassifierConfig() { const data = await apiFetch(`${API}api/classifier-config`); - if (!data) return; + if (!data) { + /* Stays disabled; only the placeholder label flips, the option itself + is kept (a later success reuses the same select). */ + document.getElementById('classifier-mode-select').disabled = true; + document.getElementById('classifier-save-btn').disabled = true; + const placeholder = document.querySelector('#classifier-mode-select option[value=""]'); + if (placeholder) placeholder.textContent = 'unavailable'; + showClassifierConfigError('classifier config unavailable; controls disabled'); + return; + } _classifierConfigData = data; + /* saveClassifierConfig re-awaits this after every successful save, so the + placeholder may already be gone by the second run -- guard, don't crash. */ + document.querySelector('#classifier-mode-select option[value=""]')?.remove(); document.getElementById('classifier-mode-select').value = data.mode; + document.getElementById('classifier-mode-select').disabled = false; + document.getElementById('classifier-save-btn').disabled = false; document.getElementById('classifier-mode-fields').innerHTML = classifierModeFieldsHtml(data.mode, data); renderClassifierResolvedPrimary(data); renderClassifierCandidateCategories(data); diff --git a/tests/test_admin_frontend.py b/tests/test_admin_frontend.py index 362df09..6735a34 100644 --- a/tests/test_admin_frontend.py +++ b/tests/test_admin_frontend.py @@ -563,3 +563,92 @@ def test_the_home_page_no_longer_renders_a_verdict_bar(): assert "VERDICT_OFF_SCALE" not in html assert "renderVerdict" not in html assert "failing, last 7 days" in html, "the fact itself still has to be on the page" + + +def _classifier_mode_select_html() -> str: + """The classifier card's mode ]*id=\"classifier-mode-select\".*?", html, re.DOTALL) + assert select, "classifier-mode-select block not found in controls.html" + return select.group(0) + + +def test_classifier_mode_select_starts_disabled(): + """The mode select ships disabled and waits for the config fetch. + + A live select during load would let a change fire onClassifierModeChange + against an empty _classifierConfigData; loadClassifierConfig is the only + thing allowed to unlock the card. + """ + html = (ROOT / "admin" / "frontend" / "controls.html").read_text() + tag = re.search(r" + `; + } + if (ENUM_VALUES[knob]) { + return enumSelect(knob, runtime, { onchangeExpr: `toggleKnob('${knob}', this.value)` }); + } + if (typeof persisted === 'number') { + // Blank posts null, NOT 0. `Number('')` is 0 in JavaScript and 0 is a + // real, costly setting on both numeric knobs, so clearing a field must + // never be read as choosing it. What null MEANS is the server's call. + const b = NUMBER_BOUNDS[knob] || {}; + const attrs = [ + b.min !== undefined ? `min="${b.min}"` : '', + b.max !== undefined ? `max="${b.max}"` : '', + b.step !== undefined ? `step="${b.step}"` : '', + b.placeholder ? `placeholder="${escapeHtml(b.placeholder)}"` : '', + ].filter(Boolean).join(' '); + const shown = (runtime === null || runtime === undefined) ? '' : String(runtime); + return ``; + } + if (typeof persisted === 'string') { + return ``; + } + return `${escapeHtml(runtimeStr)}`; +} + +function persistedControlHtml(key, entry, dirtyValue) { + // `dirtyValue` is the user's current input on a re-render that would + // otherwise reset it (loadControls re-runs after every toggle and ~1.5s + // after a save); undefined means render from the payload value. + const dirty = dirtyValue !== undefined && dirtyValue !== null; + const val = dirty + ? (typeof entry.value === 'boolean' ? dirtyValue === 'true' : dirtyValue) + : entry.value; + let control; + if (typeof val === 'boolean') { + // A switch, matching the Live column -- the bare checkbox here was the + // one control on the page that didn't look like the others. + control = `
+ +
`; + } else if (key === 'routing.default_profile') { + control = profileSelect(val); + } else if (ENUM_VALUES[key]) { + control = enumSelect(key, val, { dataAttr: 'data-config-input' }); + } else { + control = ``; + } + // Units the config file expresses but the key name doesn't, plus the + // knob's consequence note, stay beside the persisted control. + const hints = [ + UNITS[key] ? escapeHtml(UNITS[key]) : '', + noteBadge(key), + ].filter(Boolean).join(' '); + return `${control}${hints ? `${hints}` : ''}`; +} + +function knobRowHtml(row) { + const rt = row.runtimeItem; + const entry = row.persistedItem; + const keyCell = row.drift + ? `${keyHtml(row.configKey)} ${driftBadge(rt)}` + : keyHtml(row.configKey); + const liveCell = rt + ? runtimeControlHtml(rt.knob, rt) + : '-'; + let persistedCell = '-'; + let layerCell = ''; + let dirtyAttrs = ''; + if (entry) { + persistedCell = persistedControlHtml(row.configKey, entry, row.dirtyValue); + // Provenance is the layer column: where the effective value came from. + let layer = ''; + if (entry.source === 'overlay') { + layer = 'overlay'; + } else if (entry.source === 'base') { + layer = 'base'; + } + layerCell = `${layer}`; + dirtyAttrs = ` data-key="${escapeHtml(row.configKey)}" data-orig="${escapeHtml(String(entry.value === null ? '' : entry.value))}"`; + } + return ` + ${keyCell} + ${liveCell} + ${persistedCell} + ${layerCell} + `; +} + +// Before a re-render: the current input of every dirty persisted row, so an +// in-flight unsaved edit survives the refetch instead of silently reverting. +function captureDirtyPersistedInputs() { + const saved = {}; + document.querySelectorAll('#config-list .setting-row[data-key]').forEach(row => { + const input = row.querySelector('[data-config-input]'); + if (!input) return; + const current = input.type === 'checkbox' ? String(input.checked) : input.value; + if (current !== row.getAttribute('data-orig')) { + saved[row.getAttribute('data-key')] = current; + } + }); + return saved; +} + +// One render path over both payloads. Either may be null (a failed fetch, +// or the runtime fetch completing before the config one): rows render from +// whatever is available, and the whole table re-renders idempotently. +function renderKnobsTable() { + const list = document.getElementById('config-list'); + const dirtyInputs = captureDirtyPersistedInputs(); + const rows = mergeKnobRows(_runtimeState, _configState); + const keys = Object.keys(rows); + if (!keys.length) { + list.innerHTML = 'No knobs to show'; + return; + } + list.innerHTML = keys.map(key => { + const row = rows[key]; + row.dirtyValue = row.persistedItem ? dirtyInputs[key] : undefined; + return knobRowHtml(row); + }).join(''); + updateDefaultProfileHint(); + markDirty(); } function renderRuntime(state) { - const el = document.getElementById('runtime-list'); - const knobs = state || {}; - const html = Object.keys(knobs).map(key => { - const knob = knobs[key]; - const persisted = knob.persisted; - const runtime = knob.runtime; - const isDiff = JSON.stringify(persisted) !== JSON.stringify(runtime); - const runtimeStr = knobDisplayValue(runtime); - - let control; - if (typeof persisted === 'boolean') { - control = `
- -
`; - } else if (ENUM_VALUES[key]) { - control = enumSelect(key, runtime, { onchangeExpr: `toggleKnob('${key}', this.value)` }); - } else if (typeof persisted === 'number') { - // Blank posts null, NOT 0. `Number('')` is 0 in JavaScript and 0 is a - // real, costly setting on both numeric knobs -- the maximum penalty on - // the challenger dial, and an out-of-range not-quite-off on the session - // cache window -- so clearing a field must never be read as choosing it. - // What null MEANS is the server's call: neutral for the dial, a refusal - // naming session_cache_enabled for the window. - const b = NUMBER_BOUNDS[key] || {}; - const attrs = [ - b.min !== undefined ? `min="${b.min}"` : '', - b.max !== undefined ? `max="${b.max}"` : '', - b.step !== undefined ? `step="${b.step}"` : '', - b.placeholder ? `placeholder="${escapeHtml(b.placeholder)}"` : '', - ].filter(Boolean).join(' '); - const shown = (runtime === null || runtime === undefined) ? '' : String(runtime); - control = ``; - } else if (typeof persisted === 'string') { - control = ``; - } else { - control = `${escapeHtml(runtimeStr)}`; - } - - // The control already shows the live value, so the only facts worth a - // second column are disagreement with the file and a consequence the - // switch cannot state for itself. - const diffBadge = isDiff - ? `file: ${escapeHtml(knobDisplayValue(persisted))}` - : ''; - const meta = [noteBadge(key), diffBadge].filter(Boolean).join(' '); - - return settingRow({ key, meta, control }); - }).join(''); - el.innerHTML = html || '
No runtime knobs
'; + _runtimeState = state; + renderKnobsTable(); } /* A knob POST that keeps the server's `detail`. The generic apiFetch swallows @@ -813,50 +969,8 @@ async function setLocalCompute(enabled) { let _profilesCache = null; function renderConfig(config) { - const list = document.getElementById('config-list'); - if (!config || !Object.keys(config).length) { - list.innerHTML = '
No config data
'; - return; - } - const html = Object.entries(config).map(([key, raw]) => { - const entry = raw || {}; - const val = entry.value; - const source = entry.source; - const isBool = typeof val === 'boolean'; - let control; - if (isBool) { - // A switch, matching Runtime Knobs — the bare checkbox here was the one - // control on the page that didn't look like the others. - control = `
- -
`; - } else if (key === 'routing.default_profile') { - control = profileSelect(val); - } else if (ENUM_VALUES[key]) { - control = enumSelect(key, val, { dataAttr: 'data-config-input' }); - } else { - control = ``; - } - - // Provenance badge is legitimate meta: it explains where the effective - // value came from without restating the value the control already shows. - let meta = [UNITS[key] ? escapeHtml(UNITS[key]) : '', noteBadge(key)].filter(Boolean).join(' '); - if (source === 'overlay') { - meta += `${meta ? ' ' : ''}overlay`; - } else if (source === 'base') { - meta += `${meta ? ' ' : ''}base`; - } - - return settingRow({ - key, - meta, - control, - dirtyAttrs: ` data-key="${escapeHtml(key)}" data-orig="${escapeHtml(String(val === null ? '' : val))}"`, - }); - }).join(''); - list.innerHTML = html; - updateDefaultProfileHint(); - markDirty(); + _configState = config; + renderKnobsTable(); } function profileSelect(currentValue) { diff --git a/src/admin.py b/src/admin.py index 4e1eb47..9f38b9f 100644 --- a/src/admin.py +++ b/src/admin.py @@ -475,6 +475,20 @@ _FLEX_VALUES = frozenset(v.value for v in FlexPreference) _PROFILE_KNOB = "active_profile" _PROFILE_PATH: tuple[str, ...] = ("routing", "default_profile") +# The dotted config.yaml path behind each runtime knob, DERIVED from the same +# registries that drive the POST handlers -- never hand-copied, so a knob +# added to any table below is automatically labelled. GET /admin/api/runtime +# ships one label per knob so the controls page can pair each runtime row +# with its persisted twin by config key alone, without re-deriving this +# mapping in JavaScript. +_RUNTIME_KNOB_PATHS: dict[str, tuple[str, ...]] = { + **_BOOL_KNOBS, + **{knob: spec[0] for knob, spec in _FLOAT_KNOBS.items()}, + **{knob: spec[0] for knob, spec in _INT_KNOBS.items()}, + _FLEX_KNOB: _FLEX_PATH, + _PROFILE_KNOB: _PROFILE_PATH, +} + # --- persisted config allowlist ------------------------------------------------- # Dotted config.yaml paths an operator is allowed to edit. Everything else — # classifier/verification/local_vision URLs and model names, api_key_env, @@ -2363,11 +2377,21 @@ def build_router( @router.get("/api/runtime") def admin_runtime_state() -> dict: - """Persisted (config.yaml) vs runtime (in-memory) value of each knob.""" + """Persisted (config.yaml) vs runtime (in-memory) value of each knob. + + Each item also carries ``config_key``, the dotted config.yaml path + behind the knob (``_RUNTIME_KNOB_PATHS``), so the controls page can + pair the runtime half of a row with its persisted half without + re-deriving the knob-name -> config-path mapping in JavaScript. + """ persisted = _runtime_state(load_config("config/config.yaml")) runtime = _runtime_state(cfg) return { - key: {"persisted": persisted[key], "runtime": runtime[key]} + key: { + "persisted": persisted[key], + "runtime": runtime[key], + "config_key": ".".join(_RUNTIME_KNOB_PATHS[key]), + } for key in persisted } diff --git a/tests/test_admin_js_units.py b/tests/test_admin_js_units.py index 3584a9d..1ced664 100644 --- a/tests/test_admin_js_units.py +++ b/tests/test_admin_js_units.py @@ -326,3 +326,135 @@ def test_stale_dismissal_pruned_and_new_warnings_open(): "assert.strictEqual(dismissed.size, 0);\n" "assert.strictEqual(openWarnings(fresh, dismissed).length, 50);" ) + + +CONTROLS_HTML = ROOT / "admin" / "frontend" / "controls.html" +MERGE_KNOB_ROWS_BEGIN = "/* MERGE_KNOB_ROWS:BEGIN */" +MERGE_KNOB_ROWS_END = "/* MERGE_KNOB_ROWS:END */" + + +def _assert_against_merge_units(assertion_js: str) -> subprocess.CompletedProcess[str]: + """Assert ``assertion_js`` against the real mergeKnobRows source. + + The units (normalizeKnobValue, knobValuesDrift, mergeKnobRows) live in + controls.html between the MERGE_KNOB_ROWS markers and are self-contained: + they take the two API payloads as arguments and touch no DOM, so the + extraction runs as-is under node. + """ + return _run_node( + "const assert = require('assert');\n" + + _extract_between(CONTROLS_HTML, MERGE_KNOB_ROWS_BEGIN, MERGE_KNOB_ROWS_END) + + "\n" + + assertion_js + ) + + +RUNTIME_BOTH_AND_ONLY = ( + "{\n" + " log_route_decisions: { persisted: true, runtime: false, config_key: 'logging.log_route_decisions' },\n" + " circuit_breaker_enabled: { persisted: true, runtime: true, config_key: 'circuit_breaker.enabled' },\n" + "}" +) +PERSISTED_BOTH_AND_ONLY = ( + "{\n" + " 'logging.level': { value: 'info', source: 'base' },\n" + " 'circuit_breaker.enabled': { value: true, source: 'base' },\n" + " 'objective.quality_tolerance': { value: 0.7, source: 'base' },\n" + "}" +) + + +@skip_without_node +def test_merge_knob_rows_row_set_is_the_union_of_both_key_sets(): + """Runtime rows pair by the config_key the API carries; persisted rows by + their own key. The row set is the union: a one-sided knob still gets a + row, with the other column empty.""" + _assert_against_merge_units( + f"const rows = mergeKnobRows({RUNTIME_BOTH_AND_ONLY}, {PERSISTED_BOTH_AND_ONLY});\n" + "assert.deepStrictEqual(Object.keys(rows).sort(), [\n" + " 'circuit_breaker.enabled',\n" + " 'logging.level',\n" + " 'logging.log_route_decisions',\n" + " 'objective.quality_tolerance',\n" + "]);\n" + "assert.strictEqual(rows['logging.log_route_decisions'].persistedItem, null);\n" + "assert.strictEqual(rows['logging.log_route_decisions'].runtimeItem.knob, 'log_route_decisions');\n" + "assert.strictEqual(rows['objective.quality_tolerance'].runtimeItem, null);\n" + "assert.strictEqual(rows['objective.quality_tolerance'].persistedItem.value, 0.7);" + ) + + +@skip_without_node +def test_merge_knob_rows_one_sided_inputs_yield_one_sided_rows(): + """Only one payload present (a failed fetch, or the runtime fetch landing + first): rows render from what is available, never from nothing.""" + _assert_against_merge_units( + "const runtimeOnly = mergeKnobRows(\n" + " { k: { persisted: true, runtime: false, config_key: 'a.enabled' } },\n" + " null\n" + ");\n" + "assert.deepStrictEqual(Object.keys(runtimeOnly), ['a.enabled']);\n" + "assert.strictEqual(runtimeOnly['a.enabled'].persistedItem, null);\n" + "const persistedOnly = mergeKnobRows(null, { 'a.enabled': { value: true, source: 'base' } });\n" + "assert.deepStrictEqual(Object.keys(persistedOnly), ['a.enabled']);\n" + "assert.strictEqual(persistedOnly['a.enabled'].runtimeItem, null);\n" + "assert.deepStrictEqual(mergeKnobRows(null, null), {});" + ) + + +@skip_without_node +def test_merge_knob_rows_drift_rule_follows_the_runtime_payload_only(): + """A row WITH a runtime item drifts when its runtime value differs from + the payload's persisted value -- both-sides rows and runtime-only rows + alike, because a restart reverts them both. A persisted-only row never + drifts: no live value exists.""" + _assert_against_merge_units( + f"const rows = mergeKnobRows({RUNTIME_BOTH_AND_ONLY}, {PERSISTED_BOTH_AND_ONLY});\n" + "assert.strictEqual(rows['logging.log_route_decisions'].drift, true);\n" + "assert.strictEqual(rows['circuit_breaker.enabled'].drift, false);\n" + "assert.strictEqual(rows['objective.quality_tolerance'].drift, false);" + ) + + +@skip_without_node +def test_merge_knob_rows_ignores_the_persisted_columns_input_value(): + """Drift is computed from the runtime payload alone. A persisted column + holding an unsaved dirty edit ('debug') must not read as drift when the + router is running exactly what a restart would load ('info').""" + _assert_against_merge_units( + "const rows = mergeKnobRows(\n" + " { k: { persisted: 'info', runtime: 'info', config_key: 'logging.level' } },\n" + " { 'logging.level': { value: 'info', source: 'base' } }\n" + ");\n" + "assert.strictEqual(rows['logging.level'].drift, false);" + ) + + +@skip_without_node +@pytest.mark.parametrize( + "persisted,runtime,expect_drift", + [ + ("0.10", 0.1, False), # numbers compare numerically + (0.1, "0.10", False), + (2, 2.0, False), + (True, False, True), + (False, True, True), + ("info", "debug", True), + (None, 5, False), # null/absent on either side is not drift + (5, None, False), + (None, None, False), + ], +) +def test_merge_knob_rows_value_normalization(persisted, runtime, expect_drift): + """Booleans as booleans, numbers numerically, null/absent never drifting.""" + def js(v: object) -> str: + # Python repr is not JavaScript: True/False/None have other names. + return {True: "true", False: "false", None: "null"}.get(v, repr(v)) + + _assert_against_merge_units( + "const rows = mergeKnobRows(\n" + f" {{ k: {{ persisted: {js(persisted)}, runtime: {js(runtime)}, config_key: 'a.knob' }} }},\n" + " {}\n" + ");\n" + f"assert.strictEqual(rows['a.knob'].drift, {str(expect_drift).lower()});" + ) diff --git a/tests/test_admin_knob_coverage.py b/tests/test_admin_knob_coverage.py index 16ced8e..8ec62aa 100644 --- a/tests/test_admin_knob_coverage.py +++ b/tests/test_admin_knob_coverage.py @@ -68,13 +68,17 @@ No database, no network, no config file is read or written. from __future__ import annotations import enum +import sqlite3 import types import typing +from pathlib import Path from pydantic import BaseModel +from starlette.testclient import TestClient import admin -from config import RouterConfig +import dispatcher +from config import RouterConfig, load_config # Clause 2 of the scope rule. A leaf with one of these names configures WHERE # the router points (endpoint, model id, credential env var, filesystem path, @@ -495,3 +499,82 @@ def test_admin_registries_point_at_real_config_knobs(): "admin registry path(s) that are not scalar fields of RouterConfig:\n " + "\n ".join(phantom) ) + + +# --- Component 6: one knob table ----------------------------------------------- +# The two tests below step outside the pure-reflection contract documented +# above: the first drives the real app (still offline -- a temp SQLite file, +# no network, no port), the second reads admin/frontend/controls.html as +# text. Both exist because the merged knob table pairs runtime rows with +# persisted rows by a config key the API labels and the frontend source +# selects on, and neither half of that contract is visible to reflection. + +ROOT = Path(__file__).resolve().parent.parent +SCHEMA_SQL = (ROOT / "config" / "schema.sql").read_text() +CONTROLS_HTML = ROOT / "admin" / "frontend" / "controls.html" + + +def test_runtime_api_labels_every_knob_with_a_real_config_key(tmp_path, monkeypatch): + """GET /admin/api/runtime labels every knob with a resolvable config path. + + The controls page pairs each runtime row with its persisted twin by + config_key alone, so a missing or mistyped label renders one-sided rows + that cannot be fixed from the UI. Walking getattr down a loaded + RouterConfig checks each label against the model itself, independently + of the registry that produced it. + """ + conn = sqlite3.connect(str(tmp_path / "test.db")) + conn.row_factory = sqlite3.Row + conn.executescript(SCHEMA_SQL) + conn.close() + + monkeypatch.setattr(dispatcher.cfg.database, "path", str(tmp_path / "test.db")) + monkeypatch.setenv("NEURALWATT_API_KEY", "test-key") + + with TestClient(dispatcher.app) as client: + resp = client.get("/admin/api/runtime") + assert resp.status_code == 200 + body = resp.json() + assert body + + cfg = load_config(str(ROOT / "config" / "config.yaml")) + for knob, item in body.items(): + assert set(item) == {"persisted", "runtime", "config_key"}, knob + # An AttributeError here names the broken label directly. + obj: typing.Any = cfg + for part in item["config_key"].split("."): + obj = getattr(obj, part) + keys = [item["config_key"] for item in body.values()] + assert len(set(keys)) == len(keys), ( + "runtime knobs sharing one config_key would fuse into one table row: " + + ", ".join(sorted(k for k in keys if keys.count(k) > 1)) + ) + assert set(body) == set(admin._RUNTIME_KNOB_PATHS) + + +def test_controls_page_keeps_the_table_selectors_its_js_depends_on(): + """controls.html still serves the strings the dependent JS sites select. + + markDirty and the save collector re-query + ``#config-list .setting-row[data-key]``, the default-profile hint looks + its row up by exact data-key, init() attaches delegated listeners by the + list's id, and the profile select is found via data-profile-select. A + rename in any of these silently kills dirty tracking, saving and the + hint. The tbody fallback must stay table-shaped -- a bare
inside + a is invalid markup the browser will hoist out of the table. + """ + html = CONTROLS_HTML.read_text(encoding="utf-8") + assert 'id="config-list"' in html + for selector in ( + '#config-list .setting-row[data-key="routing.default_profile"]', + "#config-list .setting-row[data-key]", + "getElementById('config-list')", + ): + assert selector in html, selector + # The persisted row template still marks rows for the collectors. + assert 'class="setting-row"' in html + assert " data-key=" in html + assert " data-orig=" in html + assert "data-config-input" in html + assert "data-profile-select" in html + assert '