fix(stt): preprocess .silk voice notes before transcription - #11593
fix(stt): preprocess .silk voice notes before transcription#11593dso2ng wants to merge 1 commit into
Conversation
|
Maintainer context: I did a repo search for related Related but distinct items:
So the intended scope of this PR is specifically:
|
|
Maintainer note: this fork-based PR already has focused local verification recorded in the PR description/comments, but GitHub Actions for the PR still appear to be stuck in Current blocker appears to be maintainer approval for fork workflows in the GitHub UI. If a maintainer approves / runs the PR workflows, I can help follow up on any real CI failures after the checks actually execute. |
b2fccf4 to
a788f07
Compare
|
Maintainer update: I refreshed this branch onto current |
|
Update: refreshed this branch onto current Focused verification on the refreshed branch:
Current PR state is back to mergeable on GitHub; there are still no checks reported on the branch from the fork side. |
|
Refresh update: I merged current Updated head: Focused local verification on the refreshed branch:
The PR is mergeable again. The current GitHub Actions runs on the refreshed head still show |
440794e to
4662275
Compare
|
Refresh update: I force-pushed this branch as a clean single commit on top of current Updated head: The refreshed patch keeps the current main STT dispatch behavior, including local command, managed OpenAI audio gateway, and xAI paths, while adding the Focused local verification on the refreshed branch:
GitHub metadata now reports the PR as mergeable again. There are still no checks reported on this fork branch, so CI appears not to have run yet. |
austinpickett
left a comment
There was a problem hiding this comment.
Please use .github/PULL_REQUEST_TEMPLATE.md
|
Thanks for the review note. I updated the PR description to follow |
4662275 to
e272166
Compare
|
Refresh update: I force-pushed this branch as a clean single commit on top of current Updated head: This also keeps the PR description in the current Focused local verification on the refreshed branch:
GitHub Actions on the new head are still |
|
test |
|
Hi @dso2ng, I'd like to help get this PR across the finish line. The collaborator requested using I'm also interested in extending this to cover the outbound direction (OGG → SILK for voice bubbles, #9971) once the inbound preprocessing lands. |
e272166 to
416b0e9
Compare
|
cc @austinpickett — the template feedback has been addressed and the branch is mergeable again. Thanks @ryan-flow — I appreciate the offer to help. I refreshed this PR directly rather than opening a duplicate fresh PR, since the description is already in the Updated head: What changed in this refresh:
Focused verification on the refreshed branch:
The PR is mergeable again. GitHub still reports no checks on this fork branch, so the next blocker appears to be the usual fork-workflow approval / re-review rather than a local test failure. For #9971 / outbound OGG->SILK voice bubbles: agreed that it should stay as a separate follow-up from this inbound preprocessing PR, so #32212 looks like the right place for that direction. |
|
Following up on the earlier template review request:
Could a maintainer please re-review / clear the stale template-related “changes requested” state and approve the fork workflow runs when you have a chance? I’m happy to follow up on any real CI failures once the checks are allowed to run. |
416b0e9 to
9b7a2d2
Compare
|
Refresh update: I rebased this branch onto current Updated head: Focused local verification on the refreshed head:
GitHub Actions is still blocked at the fork workflow approval gate ( cc @austinpickett when you have a chance. |
9b7a2d2 to
53234c5
Compare
|
Refresh update: I rebased this branch onto current Updated head: Targeted local verification on the refreshed head:
GitHub Actions is still at the fork workflow approval gate on the new head ( |
53234c5 to
4d4f86a
Compare
|
Refresh update: I rebased this branch onto current Updated head: Targeted local verification on the refreshed head:
GitHub metadata now reports the PR as mergeable again. The new workflow runs are still blocked at the fork workflow approval gate ( cc @austinpickett when you have a chance to re-review / clear the stale template-related changes-requested state. |
4d4f86a to
0be5d78
Compare
|
Refresh update: I rebased this branch onto current Updated head: Targeted local verification on the refreshed head:
GitHub metadata now reports the PR as mergeable again. The new workflow runs are still at the fork workflow approval gate ( cc @austinpickett when you have a chance to re-review / clear the stale template-related changes-requested state. |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the focused inbound-STT compatibility work. The premise is valid: current main rejects .silk before provider dispatch (tools/transcription_tools.py:1032-1037, 1643-1646).
Problems
tools/transcription_tools.py:1679preprocesses before validation, and the helper invokespilk.silk_to_wav()at line 1070. This permits.silksymlinks to reach the decoder before current main's symlink rejection at line 1026. Commit95848b1cbcf3549490cc413b701080d26d33e0f5explicitly established that validation must precede dispatch.- The same order means the original
.silkbypasses the current 25 MiB limit (tools/transcription_tools.py:1038-1045) until after conversion; the PR validates onlyprepared_pathat line 1691. - The new test at
tests/tools/test_transcription_tools.py:921mocks both preprocessing and validation, so it cannot cover either regression.
Suggested changes
- Validate source path safety and source size before decoding, while deferring only extension validation until the converted WAV exists.
- Add
.silksymlink and oversized-source tests that assert the converter is not called.
Automated hermes-sweeper review.
| "error": "STT is disabled in config.yaml (stt.enabled: false).", | ||
| } | ||
| try: | ||
| prepared_path, cleanup_dir, prep_error = _prepare_audio_for_transcription(file_path) |
There was a problem hiding this comment.
This invokes the decoder before the existing validation. A .silk symlink reaches pilk.silk_to_wav() before _validate_audio_file() can reject it, regressing the explicit symlink hardening from 95848b1. Split source safety/size checks from extension validation and run the former before this call.
| @@ -909,6 +917,24 @@ def test_model_override_passed_to_local(self, sample_ogg): | |||
|
|
|||
| assert mock_local.call_args[0][1] == "large-v3" | |||
|
|
|||
| def test_converts_silk_before_dispatch(self, sample_silk): | |||
| with patch("tools.transcription_tools._prepare_audio_for_transcription", | |||
There was a problem hiding this comment.
Because this test mocks both preprocessing and _validate_audio_file, it cannot detect the new decoder-before-validation ordering. Add regression coverage for a .silk symlink and an oversized .silk source that asserts preprocessing is not invoked.
0be5d78 to
e5db793
Compare
|
Thanks for the detailed review — addressed on the current head
Focused verification on the updated head: Could you please re-review the updated head? |
|
Merged into main via consolidated salvage PR #73510 (merge Your contribution is credited to you in git history. Thank you! Closing this PR as merged-via-salvage. |
What does this PR do?
This PR adds inbound
.silkvoice-note preprocessing to the STT path.Hermes can receive voice notes from messaging platforms / bridges as Silk-encoded
.silkfiles. Before this change,tools.transcription_tools.transcribe_audio()validated the file extension before any format-specific preprocessing, so.silkinputs failed early withUnsupported format: .silkand never reached the configured STT provider.After this change:
.silkinputs are detected before normal audio-extension validation..silkinputs to temporary.wavfiles through optionalpilkwhen available..wavcontinues through the existing STT provider dispatch path unchanged.Related Issue
N/A — this is a focused compatibility fix for inbound Silk voice-note transcription.
Related / adjacent context:
.silkinputs enter the STT pipeline.Type of Change
Changes Made
tools/transcription_tools.py.silkdetection before normal supported-format validation..silkinputs to a temporary.wavvia optionalpilk..wavthrough the existing provider dispatch path.tests/tools/test_transcription_tools.py.silkinputs are converted before provider dispatch.pyproject.toml/uv.lockpilkdependency under thevoiceextra so Silk preprocessing is available when voice dependencies are installed.How to Test
Focused local verification on the refreshed branch:
Run the Silk preprocessing regression:
python -m pytest tests/tools/test_transcription_tools.py::TestTranscribeAudioDispatch::test_converts_silk_before_dispatch -o 'addopts=' -qExpected:
1 passedRun the affected transcription / managed-media tests:
python -m pytest tests/tools/test_transcription_tools.py tests/tools/test_managed_media_gateways.py -o 'addopts=' -qExpected:
101 passedCheck Python syntax for the touched transcription files:
Expected: no output / exit 0
Verify the lockfile is current:
Expected: exit 0
Check diff whitespace:
Expected: no output / exit 0
Manual smoke test when a messaging gateway provides a Silk file:
Unsupported format: .silk.Checklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/Apilkdependency; no platform-specific shell commands are introduced..silkbefore dispatch.For New Skills
N/A — this PR does not add a skill.
Screenshots / Logs
N/A — no UI changes.
Relevant verification output from the refreshed branch was posted in the PR discussion: