Resolve default snd resources from package data#150
Conversation
📝 WalkthroughWalkthroughThis change adds packaged sound resolution to the PlaybackService, enabling the system to locate bundled OVOS sounds from installed packages. It introduces constants for default sound packages and aliases, implements a new resolver method for packaged sounds, and enhances the existing URI resolution to check packaged resources as a fallback. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan for PR comments
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
test/unittests/test_playback_service.py (1)
137-199: Add one case where a default sound package is missing.The resolver now depends on skipping packages that cannot be imported, but these tests always make
files()return a path. A small regression test wherefiles("ovos_audio")orfiles("ovos_dinkum_listener")raisesModuleNotFoundErrorand the other package still resolves would pin the production branch this PR is relying on.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_playback_service.py` around lines 137 - 199, Add a new unit test that exercises PlaybackService._resolve_sound_uri when one of the default sound packages cannot be imported: create temp resource(s) for the remaining package, patch ovos_audio.service.files so that calling files("ovos_audio") or files("ovos_dinkum_listener") raises ModuleNotFoundError for one package while returning a Path for the other, patch resolve_resource_file as needed to return the fallback path, call PlaybackService._resolve_sound_uri with a packaged sound name and assert it resolves to the path from the available package and that resolve_resource_file is not called (or is called only for the expected fallback case); reference the existing test helpers and patterns used in test_playback_service.py (e.g., patches around ovos_audio.service.files, ovos_audio.service.resolve_resource_file and PlaybackService._resolve_sound_uri) to add this regression test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ovos_audio/service.py`:
- Around line 486-503: Reject path traversal inputs before joining by validating
the incoming URI in _resolve_packaged_sound_uri: detect and reject any
normalized_uri (and any alias from _DEFAULT_SOUND_ALIASES) that contains '..' or
attempts to walk outside the intended res/ tree (e.g., components like "../" or
leading "/" after normalization) and return None early; apply the same
validation logic for the analogous resolver at lines 511-518 (same function or
sibling function using resource_parts and _DEFAULT_SOUND_PACKAGES) so no
candidate is passed to resource.joinpath or local os.path joins when it can
escape the res/ directory.
- Around line 497-500: The broad except around resolving package resources is
hiding real errors; narrow it to only catch the expected package-import failure
(ModuleNotFoundError and ImportError) for the
files(package).joinpath(*resource_parts) call in service.py so we continue when
the package truly isn't installed, and let other exceptions surface (either
re-raise them or log them with logger.exception before re-raising) so resolver
bugs are not swallowed.
---
Nitpick comments:
In `@test/unittests/test_playback_service.py`:
- Around line 137-199: Add a new unit test that exercises
PlaybackService._resolve_sound_uri when one of the default sound packages cannot
be imported: create temp resource(s) for the remaining package, patch
ovos_audio.service.files so that calling files("ovos_audio") or
files("ovos_dinkum_listener") raises ModuleNotFoundError for one package while
returning a Path for the other, patch resolve_resource_file as needed to return
the fallback path, call PlaybackService._resolve_sound_uri with a packaged sound
name and assert it resolves to the path from the available package and that
resolve_resource_file is not called (or is called only for the expected fallback
case); reference the existing test helpers and patterns used in
test_playback_service.py (e.g., patches around ovos_audio.service.files,
ovos_audio.service.resolve_resource_file and PlaybackService._resolve_sound_uri)
to add this regression test.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cd22ca05-12da-4dff-935f-87228beef35d
📒 Files selected for processing (2)
ovos_audio/service.pytest/unittests/test_playback_service.py
| def _resolve_packaged_sound_uri(uri: str) -> Optional[str]: | ||
| """Resolve bundled OVOS sounds from installed package resources.""" | ||
| normalized_uri = uri.replace("\\", "/") | ||
| candidates = [normalized_uri] | ||
| alias = _DEFAULT_SOUND_ALIASES.get(normalized_uri) | ||
| if alias and alias not in candidates: | ||
| candidates.append(alias) | ||
|
|
||
| for candidate in candidates: | ||
| resource_parts = ("res", *candidate.split("/")) | ||
| for package in _DEFAULT_SOUND_PACKAGES: | ||
| try: | ||
| resource = files(package).joinpath(*resource_parts) | ||
| except Exception: | ||
| continue | ||
| if resource.is_file(): | ||
| return str(resource) | ||
| return None |
There was a problem hiding this comment.
Reject snd/../... before joining any paths.
The new resolver still accepts values like snd/../../../../tmp/x.wav: the prefix check passes, then both the local os.path.join(...) path and the packaged joinpath(...) path can walk out of res/ on filesystem-backed installs. Since these URIs come from bus messages, this turns sound resolution into arbitrary local-path lookup.
Suggested hardening
+from pathlib import PurePosixPath
+
+ `@staticmethod`
+ def _normalize_sound_uri(uri: str) -> str:
+ path = PurePosixPath(uri.replace("\\", "/"))
+ if path.is_absolute() or ".." in path.parts or path.parts[:1] != ("snd",):
+ raise ValueError(f"Invalid sound URI: {uri}")
+ return path.as_posix()
+
`@staticmethod`
def _resolve_packaged_sound_uri(uri: str) -> Optional[str]:
"""Resolve bundled OVOS sounds from installed package resources."""
- normalized_uri = uri.replace("\\", "/")
+ normalized_uri = PlaybackService._normalize_sound_uri(uri)
candidates = [normalized_uri]
alias = _DEFAULT_SOUND_ALIASES.get(normalized_uri)
if alias and alias not in candidates:
candidates.append(alias) if uri.startswith("snd/") or uri.startswith("snd\\"):
- normalized_uri = uri.replace("\\", "/")
+ normalized_uri = PlaybackService._normalize_sound_uri(uri)
local_uri = os.path.join(os.path.dirname(__file__), "res",
normalized_uri)Also applies to: 511-518
🧰 Tools
🪛 Ruff (0.15.5)
[error] 499-500: try-except-continue detected, consider logging the exception
(S112)
[warning] 499-499: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ovos_audio/service.py` around lines 486 - 503, Reject path traversal inputs
before joining by validating the incoming URI in _resolve_packaged_sound_uri:
detect and reject any normalized_uri (and any alias from _DEFAULT_SOUND_ALIASES)
that contains '..' or attempts to walk outside the intended res/ tree (e.g.,
components like "../" or leading "/" after normalization) and return None early;
apply the same validation logic for the analogous resolver at lines 511-518
(same function or sibling function using resource_parts and
_DEFAULT_SOUND_PACKAGES) so no candidate is passed to resource.joinpath or local
os.path joins when it can escape the res/ directory.
| try: | ||
| resource = files(package).joinpath(*resource_parts) | ||
| except Exception: | ||
| continue |
There was a problem hiding this comment.
Only swallow the expected “package not installed” failure here.
Catching every Exception masks real resolver bugs and can fall through to the old None does not exist error instead of surfacing the actual packaging problem. Narrow this to the expected import failure (ModuleNotFoundError/ImportError) and log or re-raise anything else.
Suggested change
for package in _DEFAULT_SOUND_PACKAGES:
try:
resource = files(package).joinpath(*resource_parts)
- except Exception:
+ except ModuleNotFoundError:
continue
+ except Exception:
+ LOG.exception("Failed to inspect packaged sound '%s' in '%s'",
+ candidate, package)
+ raise
if resource.is_file():
return str(resource)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try: | |
| resource = files(package).joinpath(*resource_parts) | |
| except Exception: | |
| continue | |
| for package in _DEFAULT_SOUND_PACKAGES: | |
| try: | |
| resource = files(package).joinpath(*resource_parts) | |
| except ModuleNotFoundError: | |
| continue | |
| except Exception: | |
| LOG.exception("Failed to inspect packaged sound '%s' in '%s'", | |
| candidate, package) | |
| raise | |
| if resource.is_file(): | |
| return str(resource) |
🧰 Tools
🪛 Ruff (0.15.5)
[error] 499-500: try-except-continue detected, consider logging the exception
(S112)
[warning] 499-499: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ovos_audio/service.py` around lines 497 - 500, The broad except around
resolving package resources is hiding real errors; narrow it to only catch the
expected package-import failure (ModuleNotFoundError and ImportError) for the
files(package).joinpath(*resource_parts) call in service.py so we continue when
the package truly isn't installed, and let other exceptions surface (either
re-raise them or log them with logger.exception before re-raising) so resolver
bugs are not swallowed.
1 participant
Summary
snd/*system sounds from installed OVOS package resources before falling back to generic resource lookupsnd/end_listening.wav->snd/start_listening.wavandsnd/cancel.mp3->snd/error.mp3Problem
ovos-audiocan receive valid URIs likesnd/acknowledge.mp3and still fail withFileNotFoundError: None does not existwhen the actual sound files are shipped by another installed package such asovos_dinkum_listener/res/snd.Validation
python3.11 -m venv .venv311 .venv311/bin/pip install -e . pytest .venv311/bin/pytest -q test/unittests/test_playback_service.py test/unittests/test_service_handlers.pySummary by CodeRabbit
New Features
Tests