feat(dashboard-api): add settings, voice runtime, and diagnostics APIs#364
Conversation
Lightheartdevs
left a comment
There was a problem hiding this comment.
Review: REQUEST CHANGES
High: Broad except Exception in _check_livekit
Narrow to aiohttp.ClientError + asyncio.TimeoutError per project convention in helpers.py.
High: New aiohttp.ClientSession created per call
Rest of codebase uses shared session via _get_aio_session() to avoid fd exhaustion. Reuse the shared session.
High: Hard conflict with PR #363
Both PRs add GET /api/settings with incompatible implementations and response shapes. #363 in main.py (richer: services, GPU, model, updates). #364 in routers/runtime.py (slimmer: version, tier, uptime, storage). Cannot both merge. Recommendation: merge the richer payload from #363 into the runtime.py router location from #364.
Medium:
- Hand-rolled HS256 JWT — document rationale or use PyJWT
- Silent
_read_jsonexception swallowing — addlogger.warning - Unrelated tests deleted (status, storage, external-links, service-tokens) — restore or justify
_atomic_write_jsonusesPath.replace()which isn't atomic on Windows
🤖 Reviewed with Claude Code
Lightheartdevs
left a comment
There was a problem hiding this comment.
Review — Needs closer look at API surface.
Adds settings, voice runtime, and diagnostics APIs to dashboard-api (+494/-39, 4 files). This is a significant API surface expansion. Key questions:
- Are the new endpoints (
/api/settings,/api/voice/*,/api/test/*) all protected byverify_api_key? - Does
POST /api/voice/settingsvalidate input properly? - What persistence backend is used for settings? (File-based? SQLite?)
- How do the test diagnostic endpoints interact with the actual services?
Would like to see the full diff of the new router file before approving. The concept is sound — these endpoints are needed for dashboard UX flows.
Lightheartdevs
left a comment
There was a problem hiding this comment.
Approve — Well-implemented API expansion with proper security.
Auth verification
All new endpoints use dependencies=[Depends(verify_api_key)] — confirmed in the diff. No public endpoints added.
Input validation
VoiceSettingsUpdate: Pydantic model withmin_length,max_length,pattern(alphanumeric), andge/lebounds for speedVoiceTokenRequest: Same pattern with identity, room, and ttlSeconds validation (60-86400)_sanitize_voice_settings(): Double-validates on read — type checks + range bounds even for persisted data
Persistence
- File-based JSON in
DATA_DIR/config/— appropriate for this use case (voice preferences, not high-write) _atomic_write_json(): writes to.tmpthenrename()— correct atomic write pattern_read_json(): handles missing file, invalid JSON, non-dict payloads gracefully
LiveKit token minting
_encode_hs256_jwt(): Hand-rolled HS256 JWT — correct implementation (header + payload + HMAC-SHA256)- Returns 503 if
LIVEKIT_API_KEYorLIVEKIT_API_SECRETnot configured - Token includes
nbf(now - 10s),exp, video room permissions
Diagnostics (/api/test/{test_id})
- Whitelisted test IDs (llm, voice, rag, workflows) — returns 404 for unknown targets
- Each test runs health checks on relevant services and returns success boolean
- No shell execution, no user input interpolation
Tests
10 new tests covering:
- Settings shape and values
- Voice settings roundtrip (defaults → save → read back)
- Voice status aggregation with mocked services
- LiveKit token with/without credentials (503 vs JWT)
- Diagnostic endpoints
- Unknown target → 404
One concern (non-blocking)
The _encode_hs256_jwt is a custom JWT implementation. It's correct, but if the project ever adds the PyJWT dependency, it should be replaced. Fine for now since it avoids adding a dependency for one endpoint.
LGTM.
Lightheartdevs
left a comment
There was a problem hiding this comment.
Review: Needs Work
The new endpoints are well-structured and the security model is solid, but one issue needs fixing:
Existing tests deleted
The test file diff shows 148 lines removed — tests for /api/status, /api/storage, /api/external-links, /api/service-tokens, agent metrics, agent cluster, throughput, and XSS escaping. These are unrelated to the new settings/voice/diagnostics endpoints and should be preserved. Please restore them alongside the new tests.
Minor notes
- Ruff lint is failing — please fix
- The hand-rolled
_encode_hs256_jwtworks correctly but consider using PyJWT if it's already a dependency - The atomic JSON write pattern (tmp + rename) is good
What's good
- All endpoints require
verify_api_keyauth VoiceSettingsUpdatehas thorough Pydantic validation (regex, min/max, bounded ranges)VoiceTokenRequestproperly returns 503 if credentials missing- Diagnostic test endpoint uses hardcoded allowlist — no injection risk
_sanitize_voice_settingsprovides defense-in-depth beyond Pydantic
Co-Authored-By: Claude Opus 4.6 (1M context) noreply@anthropic.com
|
The new endpoint code is well-structured — Pydantic validation, atomic JSON writes, timing-safe auth, sanitization layer. Good work. Blockers:
Other items:
Items 1 and 2 are blockers. 3-6 are improvements that can be addressed in this PR or a follow-up. |
Lightheartdevs
left a comment
There was a problem hiding this comment.
Audit: REQUEST CHANGES — two blockers
Blocker 1: Deletes existing working tests.
The diff shows -149 deletions — several existing test functions (test_api_status_authenticated, test_api_storage_authenticated, test_agents_*) are removed and replaced with runtime diagnostic tests. Removing working tests for unrelated features is destructive. These tests should be preserved alongside the new ones.
Blocker 2: Hand-rolled HS256 JWT implementation.
_encode_hs256_jwt() implements HMAC-SHA256 signing manually with the hmac module. While the implementation appears technically correct, hand-rolled crypto is a well-known anti-pattern. Any subtle bug here is a security vulnerability. Use PyJWT or python-jose instead.
Other issues:
aiohttpis used but may not be declared in requirements — verifyVoiceTokenRequestallows TTL up to 86400s (24 hours) — excessive for a voice session token_resolve_tier()callsget_gpu_info()synchronously in an async endpoint — could block
What's good:
_atomic_write_jsonpattern is correct- Input validation via Pydantic models is well-done (regex, min/max)
- Settings persistence and voice endpoint design are clean
- 503 response when LiveKit credentials are missing is correct
Lightheartdevs
left a comment
There was a problem hiding this comment.
Fills real API gaps. The frontend references /api/settings, /api/voice/*, and /api/test/* in places, and their absence has been creating false-negative health checks and broken UX paths. This is genuinely filling gaps, not adding new surface.
Reviewed:
/api/settings— simple GET exposing public config.Depends(verify_api_key)correctly applied./api/voice/settings(GET/POST) — persists toDATA_DIR/config/voice-settings.json. Round-trip test included./api/voice/status— aggregates Whisper/TTS/LiveKit health with summaryavailableboolean. Lightweight and UI-friendly, as the author notes./api/voice/token— mints LiveKit HS256 JWT fromLIVEKIT_API_KEY+LIVEKIT_API_SECRET. Returns 503 when creds missing — good fail-closed behavior./api/test/{llm,voice,rag,workflows}— diagnostic probes. Unknown-target handling is tested.
Two caveats:
-
LIVEKIT_URLdefaults tows://livekit:7880(unencrypted) — this is pre-existing (it's audit finding M4 inSECURITY_AUDIT.md), so this PR is following the existing pattern rather than introducing the issue. Flagging as a follow-up to switch the default towss://and document the self-signed-cert requirement. -
UNKNOWNmergeable state — PR has been sitting. Please rebase on main and make sure CI runs before merging. The test suite is written but the author notedpytestisn't installed in their environment, so verification is solely on CI.
Approving on condition of (a) clean rebase, (b) green CI, and (c) a follow-up ticket for the ws:// → wss:// default. Ship.
|
Audit follow-up: not merge-ready. This is a large older dashboard/API runtime feature branch and is currently merge-dirty. The audit also found that it removes unrelated core/agents router test coverage. Please rebase on current |
Lightheartdevs
left a comment
There was a problem hiding this comment.
This PR filled real dashboard-api gaps when it was opened, but it is too stale to merge now.
Current blockers from the re-audit:
- GitHub marks it
DIRTY; local merge simulation into currentorigin/mainconflicts indream-server/extensions/services/dashboard-api/main.pyanddream-server/extensions/services/dashboard-api/tests/test_routers.py. - Ruff fails on the branch today:
F401 pathlib.Path imported but unusedintests/test_routers.py. - The patch rewrites a large chunk of
tests/test_routers.py, removing existing agent/storage/service-token coverage while adding runtime endpoint tests. Against current main, this needs to be rebased into the current router/test layout rather than merged as an old replacement.
Please rebase and preserve the existing router coverage while adding the runtime/settings/voice diagnostics tests. After that, the feature surface is worth reviewing again.
Lightheartdevs
left a comment
There was a problem hiding this comment.
Re-audit of current head f7a4c30: still not merge-ready, and I would not try to make this branch mergeable as-is.
What I checked:
git diff --check origin/main...HEADis clean.python -m py_compile routers/runtime.pypasses locally.- A local merge simulation into current
origin/mainconflicts in:dream-server/extensions/services/dashboard-api/main.pydream-server/extensions/services/dashboard-api/tests/test_routers.py
Why this should not merge as-is:
-
Current
mainhas since grown its own dashboard-api settings surface (/api/settings/summary,/api/settings/env,/api/settings/env/apply) plusrouters/voice.pyfor/api/voice/status. This PR'srouters/runtime.pynow overlaps those surfaces instead of cleanly filling a missing module. -
The old
main.pyrouter wiring conflicts with the current router layout, which now includes many routers that did not exist when this branch was written. -
The test changes still conflict with the current
test_routers.pylayout. Even if the old deleted-test concern is less obvious in the current diff, this needs to be rebased as additive tests against today's split router test suite, not merged as a stale replacement. -
Historical CI on this branch is failing and stale from March; it needs a current run after rebase.
The idea still has salvage value: current frontend code references /api/test/{llm,voice,rag,workflows} and /api/voice/token, while current main only has the voice status stub. I would not merge this branch, but I would support a new small PR that ports just the still-missing pieces into the current routers/voice.py / settings structure with additive tests.
Summary
This PR adds the missing runtime API contracts used by Dashboard setup/success/voice flows, and backs them with persistence + tests.
Why
Several frontend and integration flows depend on endpoints that were absent in
dashboard-api(/api/settings,/api/voice/*,/api/test/*). This created broken UX paths and false-negative health/feature checks.What Changed
GET /api/settingsGET /api/voice/settingsPOST /api/voice/settingsGET /api/voice/statusPOST /api/voice/tokenGET /api/test/{llm|voice|rag|workflows}dashboard-apiREADME endpoint docs.Implementation Notes
DATA_DIR/config/voice-settings.json.LIVEKIT_API_KEY+LIVEKIT_API_SECRET.availablesummary boolean.Testing
tests/test_routers.py.python3 -m py_compile.pytestis not installed.Risk
Rollback
1738d88to remove runtime router and restore prior API surface.