Separate speech capability controls from voice settings - #947
Conversation
Replace the duplicated Permissions speech settings controls with a single conditional link to Voice & Audio, and update localization coverage for the new surface. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Localize the Permissions voice settings link, remove obsolete removed-control resources, and stop the Permissions STT capability toggle from starting a Whisper model download. The shared card now shows setup/download guidance only when an enabled speech capability is missing required Voice & Audio setup. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Remove the STT capability toggle from Voice & Audio, add disabled-state notices that link to Permissions, and keep speech setup controls available for preconfiguration while runtime-only test and preview actions respect the capability state. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Select the Permissions voice setup warning based on the missing speech configuration: speech model, TTS voice setup, or both. Keep the shared Voice & Audio link while making the warning text specific to the enabled capabilities that still need setup. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
Codex review: needs real behavior proof before merge. Reviewed July 8, 2026, 11:56 PM ET / 03:56 UTC. Summary Reproducibility: yes. from source inspection: current defaults set Review metrics: 2 noteworthy metrics.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Maintainer decision needed
Security Review findings
Review detailsBest possible solution: Keep the Permissions and Voice & Audio ownership split, but either preserve upgrade-compatible chat-local read-aloud or get explicit maintainer approval for the new capability gate, then add direct current-head proof. Do we have a high-confidence way to reproduce the issue? Yes, from source inspection: current defaults set Is this the best way to solve the issue? No, not yet: the UI ownership split is maintainable, but the chat read-aloud gate needs an upgrade-compatible path or explicit maintainer approval plus direct current-head proof. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against c3371d66f72d. Label changesLabel justifications:
Evidence reviewedWhat I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
Review history (6 earlier review cycles)
|
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Route chat read-aloud and speaker recovery through the same TTS readiness checks used by the Permissions voice setup warning. Keep blocked unmute visually muted before showing recovery dialogs, gate overlapping speech dialogs, and apply the same readiness predicate to automatic response speech. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Keep chat TTS readiness gated on the Permissions capability only so existing Windows speech fallback can handle missing configured provider setup. Update Permissions warning copy to describe provider setup without implying all read aloud is blocked. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
shanselman
left a comment
There was a problem hiding this comment.
Reviewed and live-tested with >90% confidence. CI is green and local build passed. Manual proof: disabled STT routes Chat voice recovery to Permissions; enabled STT with missing model routes to Voice & Audio; Voice & Audio setup controls remain available while capability gating is owned by Permissions.
Summary
Change Type (select all)
Scope (select all touched areas)
winnodeLinked Issue/PR
Validation
424b484b:dotnet test .\tests\OpenClaw.Tray.Tests\OpenClaw.Tray.Tests.csproj --no-restore --filter "SpeechInputContractTests|LocalizationValidationTests|CapabilitiesPageLocalizationCoverageTests"— 26 passed, 0 skipped424b484b:$env:OPENCLAW_REPO_ROOT=(Get-Location).Path; .\build.ps1— all builds succeeded424b484b:dotnet test .\tests\OpenClaw.Shared.Tests\OpenClaw.Shared.Tests.csproj --no-restore— 2703 passed, 31 skipped424b484b:dotnet test .\tests\OpenClaw.Tray.Tests\OpenClaw.Tray.Tests.csproj --no-restore— 1577 passed, 0 skippedReal behavior proof
424b484bListening…and produced transcript text when STT was enabled/configured.Screenshots
Review notes / deferred scope
Dual-model adversarial review found no high-consensus correctness/security issues that need to expand this PR further. We intentionally deferred broader speech runtime-readiness work that predates this split:
VoiceServiceInstanceis temporarily unavailable or the model is still warming up, that should be handled by a future runtime-readiness UX pass rather than this capability/settings split.The TTS fallback concern is not deferred: this PR preserves the existing chat read-aloud fallback to Windows speech when the configured provider/voice setup is incomplete, while Permissions still warns that the selected provider needs setup.
Security Impact (required)
Yes/No): NoYes/No): NoYes/No): NoYes/No): NoYes/No): NoYes, explain risk + mitigation: N/ACompatibility / Migration
Yes/No): YesYes/No): NoYes/No): NoReview Conversations