fix(tts): unify voice resolution — dashboard /tts settings now affect the tts tool#1175
Conversation
… the tts tool (#172) The dashboard /tts page writes to system_configs[tts.<provider>.voice], but the LLM-invoked tts tool was only checking args > agent OtherConfig > builtin_tool_tenant_configs[tts].default_voice_id. Two different storage locations → the user's chosen voice was ignored, Edge defaulted to en-US-AriaNeural even when "HoaiMy" (vi-VN-HoaiMyNeural) was set correctly in the dashboard. Add system_configs as a 4th-level fallback so the dashboard becomes the single source of truth. - TtsTool gains SetSystemConfigStore(s) setter - resolveVoiceAndModel takes providerName + looks up tts.<provider>.voice/model when no higher-precedence source set - effectiveProvider resolved BEFORE voice/model so the right key is hit - Wired at gateway boot in cmd/gateway.go (after pgStores ready) - 4 unit tests covering: fallback, arg precedence, no-store, empty provider Verified against production trace 019e6036-44bb-703b-85fa-dee34f7ab2c0 where the tts tool was called with provider=edge, no voice arg, and defaulted to English instead of the configured Vietnamese voice.
|
Backlog review: merge-candidate. The PR fixes the dashboard-vs-tool TTS voice/model drift with explicit precedence and regression coverage for system_configs fallback. go and web checks are passing, and the scope is focused enough for merge review. |
|
review-pr --fix --reply result Verdict: Request changes until the prepared fix can be applied to the PR branch. Summary: The PR correctly adds Iterations: 1 review loop, 1 local fix attempt. Prepared local commit: Local verification run after the fix:
External blocker: GitHub rejected pushing to Unresolved blockers/questions: fork push permission blocked applying the prepared fix to the PR head. |
mrgoonie
left a comment
There was a problem hiding this comment.
Summary: The PR fixes the real dashboard-vs-LLM tts voice drift by using system_configs as the final voice/model fallback, with focused tests for fallback precedence. The remaining blocker is concurrency safety around the new store pointer.
Risk level: Medium
Mandatory gates:
- Duplicate/prior implementation: clear
- Project standards: issue found
- Strategic necessity: clear value
- CI/checks: green
Findings: - Critical: none
- Important:
TtsTool.systemConfigsis written underTtsTool.muinSetSystemConfigStore, but the fallback lookup path reads it without taking the same lock. Today wiring is mostly startup-time, but this creates an unnecessary race if gateway reload/reconfiguration overlaps withExecute; the resolver should snapshot the store underRLockbefore using it. - Suggestion: none
Verdict: REQUEST_CHANGES
Please apply the small mutex snapshot fix before merge. The feature itself is valuable and scoped, but this should not land with a known race in the tool execution path.
mrgoonie
left a comment
There was a problem hiding this comment.
Summary: I re-checked the latest head after the recent activity. The functional fix and tests are still good, and go test ./internal/tools passes locally, but the previous concurrency blocker is still present in this head.
Risk level: Medium
Mandatory gates:
- Duplicate/prior implementation: clear; no competing PR for the same dashboard
/tts→ tool voice fallback path was found. - Project standards: still blocked by the mutex/store-pointer access pattern already called out.
- Strategic necessity: clear value; this fixes a real dashboard-configured voice drift for LLM-invoked
ttscalls. - CI/checks: GitHub
goandwebchecks are green; localgo test ./internal/toolspassed.
Findings:
- Critical: none.
- Important:
TtsTool.systemConfigsis still written underSetSystemConfigStorewitht.mu, butresolveVoiceAndModelreadst.systemConfigsdirectly without the same lock/snapshot. Please snapshot the store underRLockbefore using it for the fallback lookup, or otherwise make the field immutable after construction with a clear guarantee. This is the same unresolved finding from the prior review. - Suggestion: none.
Verdict: COMMENT_ONLY / still blocked
Next step: apply the small mutex snapshot fix, then this should be a straightforward approve/merge candidate.
Real-world bug
A user configured Vietnamese voice "HoaiMy" (`vi-VN-HoaiMyNeural`) on the dashboard `/tts` page. The Test Playground worked. But when the LLM invoked the `tts` tool, Edge synthesized English instead.
Root cause — two TTS storage locations
When the LLM calls `tts` without an explicit `voice` arg, the resolution chain (`args > agent OtherConfig > tenant builtin`) finds no override → empty voice → Edge falls back to its built-in default (English).
Fix
Add `system_configs` as a 4th-level fallback so the dashboard is the single source of truth:
```
args > agent OtherConfig > tenant builtin > system_configs[tts..voice]
```
Tests
4 new unit tests in `internal/tools/tts_systemconfigs_fallback_test.go`:
All existing `tts` tests still pass.
Verification
Cherry-picked from fork's PR dataplanelabs#172.