Repository navigation
fix(asr): treat a rooted POSIX path as local on Windows - #2930
2 commits merged into
Conversation
`_licence_repo` decides whether an ASR model name is a local path with
`os.path.isabs(name) or name.startswith((".", "~")) or "\\" in name or
_DRIVE_PATH.match(name)`.
On Windows `os.path.isabs("/models/whisper")` is False and there is no
drive letter, so a rooted path — which Windows resolves to
`<current drive>:\models\whisper`, i.e. a local path — fell through to
`return name if "/" in name else None` and was handed back as an HF repo
id. The documented contract right above it ("recognised by syntax …
never by probing the filesystem") therefore did not hold on Windows, and
`tests/test_model_acceptance_asr.py::test_local_model_paths_are_recognised_by_syntax_not_the_filesystem`
failed there.
Because `services.model_acceptance` fails closed on an unknown repo, the
consequence was worse than a wrong classification: the user's own local
model was refused with `model_licence_required` naming their path — a
gate that can never be satisfied, since there is no repo to accept.
Accept a leading "/" as a local path too. An HF repo id never begins with
"/", so no registry identity is affected; the existing `"\\" in name`
term already covers a leading backslash. Same bug class as debpalash#1559 ("treat
/ as a separator on Windows").
|
All contributors on this pull request have signed the VoiceStudio CLA. Thank you! |
|
I have read the VoiceStudio CLA 1.0 and I hereby sign it. |
|
[Medium impact] The PR appears safe to merge.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files. (1 skipped: 1… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title uses Conventional Commit format with the asrscope, describes the main change, and the body references issue#1559.Description check The description includes all required sections and reports test results, known dependency failures, and the unrun smoke matrix. The unchecked CLA item is addressed in the PR comments, and the remainin… Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Cross-Platform Default Parity The PR changes default model-path classification, but it preserves parity across macOS, Windows, and Linux. _licence_repo()returnsNonefor/models/whisperon every platform: POSIX hosts alread…I18n Completeness (21 Locales) The pull request changes only backend ASR logic, tests, and CHANGELOG.md. The authoritative diff contains no Electron UI files, no new or changed t('...') keys, and no hardcoded Electron user-facing s… Local-First Guarantee PASS: The PR changes only _licence_repo()syntax classification, its docstring, a regression test, and the changelog. The new branch performs no network, account, API-key, analytics, or cloud operat…Backward Compatibility The PR changes only ASR licence-path classification, its test, and the changelog. It changes no omnivoice_data, voices, projects, settings, database schema, or migration files. _licence_repo()suppl…Full details: Description check
-
Explanation
The description includes all required sections and reports test results, known dependency failures, and the unrun smoke matrix. The unchecked CLA item is addressed in the PR comments, and the remaining omissions do not make the description incomplete.
Full details: Cross-Platform Default Parity
-
Explanation
The PR changes default model-path classification, but it preserves parity across macOS, Windows, and Linux.
_licence_repo()returnsNonefor/models/whisperon every platform: POSIX hosts already classify it as absolute, and the new leading-/check covers Windows. The added host-independent test verifies this for bothfaster_whispermodes, while the registry-ID assertion remains unchanged.
Full details: I18n Completeness (21 Locales)
-
Explanation
The pull request changes only backend ASR logic, tests, and CHANGELOG.md. The authoritative diff contains no Electron UI files, no new or changed t('...') keys, and no hardcoded Electron user-facing strings. Therefore the 21-locale catalog check is not applicable.
Full details: Local-First Guarantee
-
Explanation
PASS: The PR changes only
_licence_repo()syntax classification, its docstring, a regression test, and the changelog. The new branch performs no network, account, API-key, analytics, or cloud operation. Existing outbound-capable ASR code is unchanged, and the path fix returnsNonefor local paths, so it does not add an offline dependency or bypass consent gates.
Full details: Backward Compatibility
-
Explanation
The PR changes only ASR licence-path classification, its test, and the changelog. It changes no omnivoice_data, voices, projects, settings, database schema, or migration files.
_licence_repo()supplies the licence gate; returningNonefor rooted local paths allows existing local model weights without reinstalling or re-downloading them, while registry IDs remain unchanged.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 2 files. (1 skipped: 1 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
- Fix all pre-merge checks with AI
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.
Comment @coderabbitai help to get the list of available commands.
BichengWang
left a comment
BichengWang
left a comment
There was a problem hiding this comment.
Reviewed abed42c1fd8e1127bed4ea66e6947e9456d13c39 against f47221a1f0af89df27c34747471daa30391a9487.
The added guard fixes the reproduced failure: with Python 3.13's ntpath.isabs, /models/whisper produces backend_model_repos == ['/models/whisper'] and model_licence_required on the base; this head produces [] and passes ensure_backend_licence. Repo IDs, Faster-Whisper aliases, and POSIX/Windows HF snapshot paths retained their expected identities in the same checks.
One test gap remains: please make test_local_model_paths_are_recognised_by_syntax_not_the_filesystem exercise the Windows path semantics independently of the host. The current test uses the host's os.path.isabs, so all seven cases already pass on the unpatched base on macOS; the configured backend CI also runs Linux/Python 3.11. Removing the new / guard would therefore remain green there. A focused test with an isolated ab.os proxy whose path.isabs('/models/whisper') returns False would fail before this change and pass afterward without mutating the process-wide os.path. Please cover faster_whisper=True as well. This specifically affects Windows with Python 3.13+, whose ntpath.isabs rejects a single leading slash.
Validation: tests/test_model_acceptance_asr.py passed 26/26 on both base and head with HF_HUB_OFFLINE=1 and an empty HF_HUB_CACHE; the separate Python 3.13 Windows-semantics reproduction failed only on base. Native Windows execution and full suites were not run. CI and Security workflow runs for this exact head are action_required with zero jobs, so their results remain unverified.
debpalash
closed this pull request by merging all changes
into
debpalash:main
in
481cf4c
3 participants
Summary
_licence_repo()inbackend/services/asr_backend.pydecides whether an ASR model name is a local path or a Hugging Face repo id. Its local-path test readsos.path.isabs(name) or name.startswith((".", "~")) or "\\" in name or _DRIVE_PATH.match(name). On Windows a rooted path such as/models/whisperfails every one of those checks:os.path.isabs("/models/whisper")isFalse, it does not start with.or~, it carries no backslash, and it has no drive letter. It therefore fell through toreturn name if "/" in name else Noneand came back as a repo id, even though Windows resolves that exact value to<current drive>:\models\whisper, which is a local path.The damage runs past a wrong classification. The docstring directly above the function promises that local paths are "recognised by syntax … never by probing the filesystem", and
services/model_acceptancefails closed on an unknown repo, so a user's own local model was refused withmodel_licence_requirednaming their own path. That gate can never be satisfied, because there is no repository to accept. Accepting a leading/as a local path closes the one form the syntax check was missing, and it brings Windows into line with macOS and Linux, where the same input already resolves toNone.This is the same bug class as #1559, which taught path handling to treat
/as a real separator on Windows.No issue number is attached. The defect is already captured by a pinned test in this repository, so there is nothing external to link; say the word and I will open one first.
Changes
backend/services/asr_backend.py:_licence_repo()treats a name that starts with/as a local path, alongside the existing absolute,./,../,~and drive-letter checks.backend/services/asr_backend.py: the docstring now lists "a leading separator" among the recognised forms, so the documented contract matches the code on every platform.An HF repo id never begins with
/, and the"\\" in nameterm already covers a leading backslash, so no registry identity changes and no other input is affected.Type
Testing
The change is proven by the test that already guards this contract,
tests/test_model_acceptance_asr.py::test_local_model_paths_are_recognised_by_syntax_not_the_filesystem, which asserts that seven spellings of a local path all yieldNone:The whole file passes after the change (
26 passed). A wider sweep of the related suites (asr, dictation, licence, path and Windows) came back450 passed, 6 skipped, 13 failed, where all 13 failures sit intests/test_reference_asr_offline.pyand fail withModuleNotFoundError: No module named 'pydub'on a machine without that dependency. The same 13 fail with this change stashed, so they are an environment gap and not a regression from this edit.The change does not touch default behaviour on macOS or Linux, and on Windows it only affects names that are already local paths, so the
smoke-matrixregression fixture is unaffected. I did not run that matrix locally.Checklist
package.json,pyproject.toml,backend/core/version.py, and lockfilestests/fixtures/omnivoice_data/still loads green on thesmoke-matrixCI job (macOS + Windows + Linux)_licence_repo()now treats names beginning with/as local paths, so rooted POSIX paths on Windows are not classified as Hugging Face repository IDs and rejected by the model-acceptance gate. The author reports that the focused tests pass; the wider related-suite run had 13 failures attributed to missingpydub, which remains a test-environment risk to verify.