fix: harden TTS reliability, installer safety, and error surfacing#4
fix: harden TTS reliability, installer safety, and error surfacing#4yuto-trd wants to merge 2 commits into
Conversation
Address issues found in a full-project review (verified across multiple review/audit passes; build stays at 0 warnings / 0 errors on net10.0). TtsTabViewModel: - Surface synthesis/query/play failures to the user via NotificationService instead of silently logging and returning. - Add an Interlocked reentrancy guard and split out CreateQueryCore so Generate/Play no longer reset IsGenerating mid-run and no longer lose the AudioQuery to a fire-and-forget Dispatcher.Post race (they now decide on a local query value). - Make _initTcs completion idempotent (TrySetResult on all OnLoaded paths) so ReadFromJson never waits forever when VOICEVOX is missing/not loaded. VoiceVoxInstaller: - Install into a temp directory and atomically swap on success, validating the installation first, so a failed/cancelled install never leaves a broken state. - Add Zip Slip containment in ExtractZip (normalized prefix check, reject ".." and single-segment entries). - Add an idle timeout to downloads so a stalled connection cannot hang forever. VoiceVoxLoader: - Validate the installation structure (core/dict/onnxruntime/vvm), not just directory existence. - Skip a single malformed model's metas (JsonException) instead of aborting all model loading. - Centralize native library path resolution. TtsLoader: - Marshal the install notification to the UI thread, observe ContinueWith faults, and avoid the duplicate "not installed" warning on load failure. SimpleWavePlayer: - Clamp the float -> PCM16 conversion to avoid wraparound noise. Models: - Mark required JSON properties as `required` (clears CS8618 warnings). Claude-Session: https://claude.ai/code/session_01F5pRvtNeKb5nagNh83hWRz
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR hardens the VoiceVox extension across multiple layers: model properties are changed to C# ChangesVoiceVox Extension Hardening
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Beutl.Extensions.Voice/Services/VoiceVoxInstaller.cs`:
- Around line 54-60: The current code deletes the existing voicevoxHomePath
before calling MoveDirectoryCrossDevice to activate the staged directory. If
MoveDirectoryCrossDevice fails after the deletion, the user will lose their
working installation. Reorder the operations so that Directory.Delete is called
only after the MoveDirectoryCrossDevice operation completes successfully. This
ensures that if the move operation fails, the previous installation remains
intact and can still be used.
In `@src/Beutl.Extensions.Voice/Services/VoiceVoxLoader.cs`:
- Around line 42-55: The validation in line 42 uses the generic
HasDirectoryEntries method to check the open_jtalk directory, which only
verifies the directory is non-empty but does not validate that required
dictionary artifacts are present. Replace this validation by creating a new
method that specifically checks for the required dictionary files or
subdirectories within the open_jtalk folder (such as checking for required
dictionary files with specific extensions or known subdirectory names). Update
the validation logic where HasDirectoryEntries is called for the open_jtalk path
to use this new specialized method instead, ensuring that only properly
structured open_jtalk directories with all necessary artifacts are accepted as
valid.
In `@src/Beutl.Extensions.Voice/ViewModels/TtsTabViewModel.cs`:
- Around line 93-129: The OnLoaded method in TtsTabViewModel calls
_initTcs.TrySetResult() only at the end, but if voice metadata materialization
throws an exception during the LINQ operations and GroupBy call, the completion
never occurs and code waiting on _initTcs.Task will hang indefinitely. Refactor
the OnLoaded method to wrap its entire body in a try-finally block and move the
_initTcs.TrySetResult() call into the finally block to ensure completion is
guaranteed on all code paths, regardless of exceptions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 284ea368-d517-4295-b6a3-93c435e81d89
📒 Files selected for processing (7)
src/Beutl.Extensions.Voice/Models/VoiceMetadata.cssrc/Beutl.Extensions.Voice/Models/VoiceStyle.cssrc/Beutl.Extensions.Voice/Services/SimpleWavePlayer.cssrc/Beutl.Extensions.Voice/Services/VoiceVoxInstaller.cssrc/Beutl.Extensions.Voice/Services/VoiceVoxLoader.cssrc/Beutl.Extensions.Voice/TtsLoader.cssrc/Beutl.Extensions.Voice/ViewModels/TtsTabViewModel.cs
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f5f99f2983
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…eed init Follow-up to automated review feedback (CodeRabbit / Codex) on the PR: - VoiceVoxInstaller: make the staged-directory activation rollback-safe. Rename the existing install to a backup before moving the staged build into place, restore it if the move fails, and only delete the backup after a successful swap — so a failed update can no longer leave the user without a working installation. - VoiceVoxLoader: validate the open_jtalk dictionary by required artifacts (sys.dic, char.bin, matrix.bin, unk.dic) instead of only checking that the directory is non-empty, so a half-extracted dictionary is not accepted as a valid install. - TtsTabViewModel.OnLoaded: wrap the body in try/catch/finally and complete _initTcs from the finally so ReadFromJson can never hang awaiting it, even if voice-metadata materialization throws. Build: 0 warnings, 0 errors (net10.0). Claude-Session: https://claude.ai/code/session_01F5pRvtNeKb5nagNh83hWRz
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c6ec17a7fa
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| DeleteDirectoryIfExists(backupVoicevoxHomePath); | ||
| backupVoicevoxHomePath = null; |
There was a problem hiding this comment.
In the replace-existing-install path, the backup is deleted immediately after the filesystem swap, before TtsLoader.StaticLoad() verifies that the new native libraries and models can actually be loaded. If the new installation is structurally valid but Load() fails (for example due to an incompatible native library or bad model metadata), the catch path records the error but can no longer restore the previously working install, leaving the user stuck on a failing replacement. Keep the backup until after StaticLoad() succeeds and restore it on load failure.
Useful? React with 👍 / 👎.
| catch (JsonException ex) | ||
| { | ||
| _logger.LogError(ex, "Failed to deserialize VoiceMetadata: {Path}", path); | ||
| voiceModel.Dispose(); | ||
| continue; |
There was a problem hiding this comment.
When every non-song .vvm has malformed metadata, this new JsonException path skips each model and the method still falls through to InitializationTcs.TrySetResult(true)/IsLoaded = true with an empty VoiceSets. Because the install validator only requires that at least one .vvm file exists, a corrupted or incompatible model set is reported as loaded and the UI becomes enabled with no selectable voices instead of surfacing the bad install; fail the load if no usable voice metadata remains after skipping malformed models.
Useful? React with 👍 / 👎.
1 participant
Summary
Hardening pass over the whole extension, driven by a full-project review. Each finding went through a multi-layer process: dimension-based review → adversarial verification → independent false-positive audit → implementation → code review → final audit. The branch builds clean (0 warnings / 0 errors, net10.0).
Two confirmed false positives from the review were deliberately excluded, and large/separate items (download integrity verification, resampler no-op cleanup, render-time allocations) were left out of scope and are noted as follow-ups.
Changes
Reliability (
TtsTabViewModel)NotificationServiceinstead of silently logging and returning.Interlockedreentrancy guard and split outCreateQueryCoresoGenerate/Playno longer resetIsGeneratingmid-run and no longer lose theAudioQueryto a fire-and-forgetDispatcher.Postrace (they now branch on a local query value)._initTcscompletion idempotent (TrySetResulton allOnLoadedpaths) soReadFromJsoncannot wait forever when VOICEVOX is missing or not loaded.Installer safety (
VoiceVoxInstaller/VoiceVoxLoader).vvm) instead of just directory existence.ExtractZip(normalized prefix check, reject..and single-segment entries).JsonException) instead of aborting all model loading; centralize native library path resolution.Notifications / threading (
TtsLoader)ContinueWithfaults, and avoid the duplicate "not installed" warning when load actually failed with an error.Audio / models
float→ PCM16 conversion inSimpleWavePlayerto avoid wraparound noise.requiredonVoiceMetadata/VoiceStyle(clears the CS8618 warnings).Out of scope (follow-ups)
WdlResamplingSampleProviderconfigured as a no-op (input rate == output rate).AccentPhrasePanel.Render.Testing
dotnet build src/Beutl.Extensions.Voice/Beutl.Extensions.Voice.csproj -c Release→ Build succeeded, 0 warnings, 0 errors.https://claude.ai/code/session_01F5pRvtNeKb5nagNh83hWRz
Summary by CodeRabbit