chore: Add comprehensive API documentation and unit tests for plugin systems#376
Merged
chore: Add comprehensive API documentation and unit tests for plugin systems#376
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughComprehensive refactor adding extensive documentation for plugin system APIs, improving type annotations throughout the codebase, modernizing build configuration from setup.py to pyproject.toml, refactoring GitHub Actions workflows to use reusable automation, and significantly expanding test coverage with hundreds of new unit tests across templates and utilities. Changes
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
✨ Finishing Touches🧪 Generate unit tests (beta)
|
Contributor
|
Note Docstrings generation - SUCCESS |
Docstrings generation was requested by @JarbasAl. The following files were modified: * `ovos_plugin_manager/installation.py` * `ovos_plugin_manager/plugin_entry.py` * `ovos_plugin_manager/stt.py` * `ovos_plugin_manager/templates/gui.py` * `ovos_plugin_manager/templates/phal.py` * `ovos_plugin_manager/templates/stt.py` * `ovos_plugin_manager/templates/transformers.py` * `ovos_plugin_manager/templates/tts.py` * `ovos_plugin_manager/templates/vad.py` * `ovos_plugin_manager/tts.py` * `ovos_plugin_manager/utils/config.py` * `ovos_plugin_manager/wakewords.py` * `test/unittests/test_installation.py` * `test/unittests/test_pipeline.py` * `test/unittests/test_plugin_entry.py` * `test/unittests/test_solver.py` * `test/unittests/test_stt.py` * `test/unittests/test_transformers.py` * `test/unittests/test_utils.py` * `test/unittests/test_vad.py` These files were kept as they were: * `ovos_plugin_manager/templates/hotwords.py` * `test/unittests/test_g2p.py` * `test/unittests/test_language.py` * `test/unittests/test_phal.py` * `test/unittests/test_tts.py` * `test/unittests/test_wakewords.py` These file types are not supported: * `docs/api/agents.md` * `docs/api/language.md` * `docs/api/microphone.md` * `docs/api/phal.md` * `docs/api/pipeline.md` * `docs/api/stt.md` * `docs/api/transformers.md` * `docs/api/tts.md` * `docs/api/vad.md` * `docs/api/wake-word.md` * `docs/configuration.md` * `docs/index.md` * `docs/installation.md` * `docs/plugin-types.md` * `docs/writing-plugins.md`
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
ovos_plugin_manager/wakewords.py (1)
137-142:⚠️ Potential issue | 🟡 MinorMissing directory existence check may raise
FileNotFoundError.
os.listdir(WW_FOLDER)will raiseFileNotFoundErrorif the directory doesn't exist. Consider adding a guard.🛡️ Suggested fix
ww_ids = {} for lang in get_ww_supported_langs(): WW_FOLDER = f"{xdg_data_home()}/OPM/ww_configs/{lang}" + if not os.path.isdir(WW_FOLDER): + continue for voice in os.listdir(WW_FOLDER): with open(f"{WW_FOLDER}/{voice}") as f: ww_ids[voice] = json.load(f) return ww_ids🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovos_plugin_manager/wakewords.py` around lines 137 - 142, The loop in the wakewords discovery code (inside the logic that iterates get_ww_supported_langs()) calls os.listdir(WW_FOLDER) without checking that WW_FOLDER exists, which can raise FileNotFoundError; update the code that builds WW_FOLDER (used when populating ww_ids) to skip non-existent directories by guarding with os.path.isdir(WW_FOLDER) or os.path.exists(WW_FOLDER) (or wrap os.listdir in a try/except OSError) so you only call os.listdir when the folder exists and silently continue (or log a warning) if it does not.ovos_plugin_manager/tts.py (2)
139-146:⚠️ Potential issue | 🟠 Major
get_voices()breaks before the first scan and returns the wrong keys.Line 144 assumes every supported language already has a cache directory, so
get_voices()raisesFileNotFoundErroron a fresh install unless the caller remembered to pre-scan. It also uses the filename, including.json, as the dict key even though the helper/documentation describe avoice_idkey.💡 Suggested hardening
def get_voices(scan: bool = False) -> dict: if scan: scan_voices() voice_ids = {} for lang in get_tts_supported_langs(): VOICES_FOLDER = f"{xdg_data_home()}/OPM/voice_configs/{lang}" + if not os.path.isdir(VOICES_FOLDER): + continue for voice in os.listdir(VOICES_FOLDER): - with open(f"{VOICES_FOLDER}/{voice}") as f: - voice_ids[voice] = json.load(f) + if not voice.endswith(".json"): + continue + with open(f"{VOICES_FOLDER}/{voice}", encoding="utf-8") as f: + voice_ids[voice[:-5]] = json.load(f) return voice_ids🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovos_plugin_manager/tts.py` around lines 139 - 146, get_voices() currently assumes cache directories exist and uses filenames as dict keys, causing FileNotFoundError on fresh installs and wrong keys; update get_voices() to call scan_voices() when scan is requested (or ensure scan is run earlier), then iterate supported langs but guard against missing VOICES_FOLDER by checking os.path.isdir(VOICES_FOLDER) and skipping if absent, and when loading each JSON file use the loaded object's "voice_id" value as the dict key (falling back to the filename without extension if missing); also skip non-file entries and catch JSON errors to avoid crashing.
109-125:⚠️ Potential issue | 🟠 Major
scan_voices()leaves stale cache entries behind.This only overwrites voices discovered in the current run. If a plugin is uninstalled, renamed, or drops a voice, the old JSON file survives and
get_voices(scan=True)will still return it from disk. The scan step needs to reconcile or clear the existing cache before writing the new snapshot.🧹 Example refresh step before writing the new snapshot
def scan_voices() -> dict: + cache_root = os.path.join(xdg_data_home(), "OPM", "voice_configs") + if os.path.isdir(cache_root): + for lang_name in os.listdir(cache_root): + lang_dir = os.path.join(cache_root, lang_name) + if os.path.isdir(lang_dir): + for filename in os.listdir(lang_dir): + if filename.endswith(".json"): + os.remove(os.path.join(lang_dir, filename)) voice_ids = {} for lang in get_tts_supported_langs(): VOICES_FOLDER = f"{xdg_data_home()}/OPM/voice_configs/{lang}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovos_plugin_manager/tts.py` around lines 109 - 125, The scan_voices() flow currently writes discovered voice JSONs but leaves old files in VOICES_FOLDER, so update scan_voices() to reconcile the cache before writing: for each language folder (VOICES_FOLDER) compute the new set of voice IDs from get_tts_lang_configs(...) and then remove any existing files in VOICES_FOLDER that are not in that new set (or alternatively clear the folder entirely) before writing new JSONs; reference VOICES_FOLDER, get_tts_supported_langs(), get_tts_lang_configs(..., include_dialects=True), get_voice_id(plug, lang, voice), and voice_ids to locate where to perform the deletion/cleanup.
🧹 Nitpick comments (10)
docs/api/stt.md (1)
12-14: Minor documentation inconsistency with code.The return type here shows
-> dict, but the code inovos_plugin_manager/stt.pyspecifies-> Dict[str, Type[STT]]. Consider updating for consistency with the typed API.📝 Suggested fix
-### `find_stt_plugins() -> dict` +### `find_stt_plugins() -> Dict[str, Type[STT]]`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/api/stt.md` around lines 12 - 14, Update the docs signature for find_stt_plugins to match the typed API: change the return type from `-> dict` to `-> Dict[str, Type[STT]]`, and mention the mapping of entry point names to STT classes; reference the function name `find_stt_plugins` and the implementation in `ovos_plugin_manager/stt.py` to ensure consistency with the code's type hints.test/unittests/test_wakewords.py (1)
1-1: Unused import:jsonThe
jsonmodule is imported but not directly used. Thejson.loadis patched insidetest_get_wwsviapatch("json.load", ...), which doesn't require this top-level import.🧹 Proposed fix
-import json import unittest🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_wakewords.py` at line 1, Remove the unused top-level import "import json" from the test file; the test "test_get_wws" already patches json.load via patch("json.load", ...) so the module-level import is unnecessary—delete the "import json" line to clean up the unused import.ovos_plugin_manager/plugin_entry.py (1)
232-236: Consider case-insensitive GitHub URL check.The check
"github" in self.urlis case-sensitive. URLs like"https://GitHub.com/..."would not match, though this is rare in practice.🧹 Optional fix for case-insensitive check
- if self.url and "github" in self.url: + if self.url and "github" in self.url.lower(): return pip_install("git+" + self.url)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovos_plugin_manager/plugin_entry.py` around lines 232 - 236, The GitHub URL check in plugin_entry.py is case-sensitive and can miss URLs like "https://GitHub.com/..."; update the conditional that inspects self.url (the branch that calls pip_install with a "git+" URL) to perform a case-insensitive check (e.g., by lowercasing or casefolding self.url before searching for "github") so that URLs with mixed case still match; keep the existing guard that self.url is truthy and still call pip_install("git+" + self.url) when matched.test/unittests/test_pipeline.py (2)
4-4: Unused importPluginConfigTypes.
PluginConfigTypesis imported but never used in the tests.🧹 Proposed fix
-from ovos_plugin_manager.utils import PluginTypes, PluginConfigTypes +from ovos_plugin_manager.utils import PluginTypes🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_pipeline.py` at line 4, Remove the unused import PluginConfigTypes from the import statement in test/unittests/test_pipeline.py; keep only the used symbol PluginTypes in the from ovos_plugin_manager.utils import ... line so the test module no longer imports an unused name (locate the import that currently references PluginConfigTypes alongside PluginTypes).
11-49: Nested class pattern for lazy imports is functional but unconventional.The
class _ConcretePipeline:wrapper around_Implis used to defer the import ofPipelinePlugin. While this works, a simpler approach would be to use module-level conditional imports or move the import inside a function/method. This is a minor style preference and doesn't affect correctness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_pipeline.py` around lines 11 - 49, Replace the nested-class lazy-import pattern by importing or referencing the base classes directly and defining the impl classes at module level: import PipelinePlugin and ConfidenceMatcherPipeline (or move those imports into the match/match_high/match_medium/match_low methods) and declare _ConcretePipeline._Impl and _ConcreteConfidenceMatcher._Impl as simple subclasses of those bases (or assign _ConcretePipeline = _Impl and _ConcreteConfidenceMatcher = _Impl) so you remove the outer wrapper classes while preserving the same behavior; adjust references to _Impl, _ConcretePipeline and _ConcreteConfidenceMatcher accordingly.test/unittests/test_tts.py (1)
217-217: Unusedjsonimport.The
jsonimport on line 217 is not used sincejson.loadis patched directly viapatch("json.load", ...)on line 224.🧹 Remove unused import
def test_get_voices(self, xdg, mock_open, listdir, supported_langs): - import json from ovos_plugin_manager.tts import get_voices🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_tts.py` at line 217, Remove the unused top-level import json from test_tts.py (the import on the line that currently reads "import json") because json.load is being patched directly via patch("json.load", ...) in the test; simply delete that import to eliminate the unused-symbol warning and keep the test file clean.ovos_plugin_manager/templates/gui.py (1)
218-240: Consider usingClassVartype hint for the template handlers mapping.To properly annotate the class-level constant and satisfy type checkers, you could use
ClassVar:🔧 Suggested improvement
+from typing import ClassVar + class AbstractGUIPlugin: ... - _TEMPLATE_HANDLERS: Dict[str, str] = { + _TEMPLATE_HANDLERS: ClassVar[Dict[str, str]] = {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovos_plugin_manager/templates/gui.py` around lines 218 - 240, The class-level mapping _TEMPLATE_HANDLERS is currently typed as Dict[str, str]; change its annotation to use typing.ClassVar (e.g., ClassVar[Dict[str, str]]) so type checkers know it’s a true class constant; update the import to include ClassVar if missing and adjust the _TEMPLATE_HANDLERS declaration accordingly in the gui.py file (referencing the _TEMPLATE_HANDLERS symbol and the module-level typing imports).docs/writing-plugins.md (1)
202-205: Placeholder infinite loop may confuse readers.The
while True: passloop is a skeleton, but readers might not realize this needs actual implementation. Consider adding a comment or showing a minimal realistic pattern.📝 Suggested improvement
def run(self): # main daemon loop while True: - pass + # poll hardware, emit bus messages, etc. + time.sleep(1)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/writing-plugins.md` around lines 202 - 205, The placeholder infinite loop in the run method (the while True: pass inside def run(self)) is confusing; replace it with a minimal realistic skeleton or explanatory comment—e.g., show a simple daemon loop that sleeps between iterations and checks a shutdown condition or processes a task queue, or at minimum replace pass with a clear comment like "# TODO: implement main loop (process tasks, handle shutdown)" and demonstrate using time.sleep or a stop flag to avoid busy-waiting.test/unittests/test_installation.py (1)
2-2: Unused importcall.The
callimport fromunittest.mockis not used in any test method.🧹 Proposed fix
-from unittest.mock import patch, MagicMock, call +from unittest.mock import patch, MagicMock🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_installation.py` at line 2, Remove the unused import symbol `call` from the top-level import statement so only actually used mocks are imported; update the line importing from unittest.mock (where `patch, MagicMock, call` are listed) to drop `call` (leaving `patch` and `MagicMock`) to eliminate the unused import warning.test/unittests/test_transformers.py (1)
243-281: UsePLUGIN_TYPE_MAPfor the assertions or trim it down.The table includes
ttsandintent, but the class only asserts fourfind_*helpers. That makes the map easy to misread as full coverage and easy to let drift. A small loop withsubTest()over the map would keep the cases and assertions aligned.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_transformers.py` around lines 243 - 281, The tests in TestTransformerPluginUtils currently hardcode four find_* tests while PLUGIN_TYPE_MAP contains six entries (including "tts" and "intent"); update the tests to iterate PLUGIN_TYPE_MAP with a loop and subTest so assertions and cases stay in sync: for each key, import the corresponding finder function (e.g., find_utterance_transformer_plugins, find_metadata_transformer_plugins, find_audio_transformer_plugins, find_dialog_transformer_plugins, find_tts_transformer_plugins, find_intent_transformer_plugins) inside the loop, call it while patching ovos_plugin_manager.utils.find_plugins, and assert mock_find.assert_called_once_with(the PluginTypes value from PLUGIN_TYPE_MAP). Alternatively, if only the four finders should exist, remove the unused "tts" and "intent" entries from PLUGIN_TYPE_MAP to keep the map accurate.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/api/pipeline.md`:
- Around line 58-59: The docs text claims ConfidenceMatcher-style plugins are
"Used by stages like converse, common-QA, etc." which conflicts with the actual
example plugins in the ConfidenceMatcherPipeline docstring; update the docs to
match the implementation by replacing the example plugin names with the ones
from the ConfidenceMatcherPipeline docstring (e.g., adapt, padatious) or
alternately change the ConfidenceMatcherPipeline docstring to list the intended
stages; reference ConfidenceMatcherPipeline and PipelinePlugin when making the
edit so the examples are consistent.
In `@docs/api/tts.md`:
- Around line 68-72: Update the docs for OVOSTTSFactory.create to list its
actual failure modes: state that it raises ValueError if module is not set,
RuntimeError only for the explicit "unknown plugin" branch, propagates
import-related exceptions (e.g., ImportError/ModuleNotFoundError) when plugin
imports fail, and re-raises whatever exceptions come from validator.validate()
(so callers should expect validator-specific errors rather than always a
RuntimeError).
In `@docs/api/vad.md`:
- Around line 63-67: The docs incorrectly list AudioFrame.duration as an int in
seconds; update the attribute to reflect the actual unit and type—if duration is
stored in seconds, change `duration` Type to `float` and the Description to
something like "Duration of the frame in seconds (e.g., 0.03 for a 30 ms
frame)"; alternatively, if the code uses milliseconds, change the unit text to
"milliseconds" and keep Type as `int` (or `float` if fractional ms are
possible). Locate and modify the `AudioFrame.duration` entry in the table so the
Type and Description match the implementation.
In `@docs/configuration.md`:
- Around line 148-151: Update the incorrect phrasing that says a dialect match
"makes it slightly less preferred than an exact locale match": change it to
state that since entries are sorted ascending and factory methods iterate from
the end, a dialect match (which boosts priority by +15 from the default 60 to
75) makes the entry more preferred than the default, but still less preferred
than an exact locale match if that exact match has an even higher priority
value; specifically replace the sentence about "less preferred" with this
corrected description referencing the default priority (60), the dialect boost
(+15), and the iteration-from-end behavior.
In `@docs/plugin-types.md`:
- Line 16: Update the table so its plugin group strings exactly match the actual
entry-point map constants: use the canonical group name used in the entry-point
map (replace inconsistent uses of `opm.VAD`/`opm.vad` with the single canonical
key), ensure the engine class column uses `VADEngine`, and add the missing
deprecated entry `ovos.plugin.audio2ipa` (as listed in `DEPRECATED_ENTRYPOINTS`
in `ovos_plugin_manager/utils/__init__.py`); also audit and fix the same
inconsistencies on lines 83-106 so all table rows mirror the real entry-point
map.
In `@docs/writing-plugins.md`:
- Around line 45-47: The available_languages method is decorated with `@property`
but accepts cls (mismatched decorator); change the decorator to `@classmethod` (or
alternatively remove cls and use self with `@property`). Specifically update the
available_languages declaration so it uses `@classmethod` and keeps the signature
def available_languages(cls) -> Set[str]: returning {"en-US"} to match
class-level usage.
- Around line 21-23: The available_languages property is defined with a cls
parameter but uses `@property` (which expects self); either make it an instance
property by renaming cls to self in available_languages or, if class-level
access is intended like runtime_requirements, replace `@property` with
`@classproperty` and keep cls. Update the decorator and/or parameter accordingly
so available_languages matches the intended scope and mirrors the
runtime_requirements pattern for class properties.
In `@ovos_plugin_manager/installation.py`:
- Around line 71-86: The pip_install function creates the subprocess without
stdout/stderr pipes when print_logs is True but later unconditionally reads
proc.stdout/proc.stderr on failure; update pip_install so the failure path only
tries to read stdout/stderr if they were captured (i.e. if subprocess was
started with stdout=PIPE/stderr=PIPE), otherwise call
proc.wait()/proc.communicate appropriately or construct a clear error message
without dereferencing proc.stdout/proc.stderr; specifically adjust the
subprocess.Popen invocation and the error handling around proc.returncode/raise
PipException in pip_install to either always capture output (use PIPE) or guard
accesses to proc.stdout/proc.stderr before using them.
- Around line 18-30: search_pip currently can exceed max_results because the
recursive call at the end does not account for already-yielded items; fix
search_pip by tracking the total yielded and passing the remaining budget into
the recursive invocation (or by computing remaining = max_results -
yielded_count and calling search_pip(query, strict, page+1, remaining)) and
ensure you stop yielding once remaining <= 0; update the recursion site in
search_pip so the remaining max_results is decremented and checked before
recursing.
In `@ovos_plugin_manager/templates/tts.py`:
- Around line 265-268: Replace the bare except around
quebra_frases.sentence_tokenize(sentence) with an except Exception to avoid
catching SystemExit/KeyboardInterrupt, and add a logged warning that includes
the exception and the sentence before returning the fallback
sentence.split("\n"); update the try/except block around
quebra_frases.sentence_tokenize in tts.py (the tokenization/fallback logic) to
explicitly log the error and the fact you are using the newline-split fallback.
In `@ovos_plugin_manager/templates/vad.py`:
- Around line 21-26: Update the class docstring for the VAD base to avoid
overstating guarantees: state that extract_speech() uses a sliding ring-buffer
to trim leading/trailing silence but only returns a trimmed buffer once it has
observed sufficient trailing unvoiced frames and may return None if the input
ends while still triggered; mention that subclasses must implement is_silence()
and that callers should handle a None return from extract_speech().
In `@ovos_plugin_manager/utils/__init__.py`:
- Line 58: PluginTypes exposes GUI_ADAPTER but there's no corresponding
PluginConfigTypes.GUI_ADAPTER, causing asymmetry; add a matching enum member
named GUI_ADAPTER to PluginConfigTypes (or remove/make GUI_ADAPTER non-public)
so config-loading logic can treat this type uniformly—update the
PluginConfigTypes enum to include GUI_ADAPTER to mirror PluginTypes.GUI_ADAPTER.
In `@ovos_plugin_manager/utils/config.py`:
- Around line 177-181: load_plugin_configs(...) can return None so the code that
does plug_configs = {standardize_lang_tag(k): conf for k, conf in
plug_configs.items()} will crash; guard the value returned by
load_plugin_configs(plug, PluginConfigTypes(f"{plug_type.value}.config")) by
treating None as an empty dict (e.g. assign plug_configs =
load_plugin_configs(...) or {}), then run the dict comprehension using that safe
plug_configs and keep using standardize_lang_tag and PluginConfigTypes as-is to
locate language configs.
In `@ovos_plugin_manager/wakewords.py`:
- Around line 123-125: The code raises NotImplementedError before returning
ww_ids, making the subsequent return unreachable; either implement the wake word
metadata reporting logic or remove the NotImplementedError so ww_ids can be
returned—locate the block that defines ww_ids in
ovos_plugin_manager/wakewords.py and either replace the raise with the actual
implementation or delete the raise and let the function return ww_ids (or raise
only when truly unimplemented with no return).
---
Outside diff comments:
In `@ovos_plugin_manager/tts.py`:
- Around line 139-146: get_voices() currently assumes cache directories exist
and uses filenames as dict keys, causing FileNotFoundError on fresh installs and
wrong keys; update get_voices() to call scan_voices() when scan is requested (or
ensure scan is run earlier), then iterate supported langs but guard against
missing VOICES_FOLDER by checking os.path.isdir(VOICES_FOLDER) and skipping if
absent, and when loading each JSON file use the loaded object's "voice_id" value
as the dict key (falling back to the filename without extension if missing);
also skip non-file entries and catch JSON errors to avoid crashing.
- Around line 109-125: The scan_voices() flow currently writes discovered voice
JSONs but leaves old files in VOICES_FOLDER, so update scan_voices() to
reconcile the cache before writing: for each language folder (VOICES_FOLDER)
compute the new set of voice IDs from get_tts_lang_configs(...) and then remove
any existing files in VOICES_FOLDER that are not in that new set (or
alternatively clear the folder entirely) before writing new JSONs; reference
VOICES_FOLDER, get_tts_supported_langs(), get_tts_lang_configs(...,
include_dialects=True), get_voice_id(plug, lang, voice), and voice_ids to locate
where to perform the deletion/cleanup.
In `@ovos_plugin_manager/wakewords.py`:
- Around line 137-142: The loop in the wakewords discovery code (inside the
logic that iterates get_ww_supported_langs()) calls os.listdir(WW_FOLDER)
without checking that WW_FOLDER exists, which can raise FileNotFoundError;
update the code that builds WW_FOLDER (used when populating ww_ids) to skip
non-existent directories by guarding with os.path.isdir(WW_FOLDER) or
os.path.exists(WW_FOLDER) (or wrap os.listdir in a try/except OSError) so you
only call os.listdir when the folder exists and silently continue (or log a
warning) if it does not.
---
Nitpick comments:
In `@docs/api/stt.md`:
- Around line 12-14: Update the docs signature for find_stt_plugins to match the
typed API: change the return type from `-> dict` to `-> Dict[str, Type[STT]]`,
and mention the mapping of entry point names to STT classes; reference the
function name `find_stt_plugins` and the implementation in
`ovos_plugin_manager/stt.py` to ensure consistency with the code's type hints.
In `@docs/writing-plugins.md`:
- Around line 202-205: The placeholder infinite loop in the run method (the
while True: pass inside def run(self)) is confusing; replace it with a minimal
realistic skeleton or explanatory comment—e.g., show a simple daemon loop that
sleeps between iterations and checks a shutdown condition or processes a task
queue, or at minimum replace pass with a clear comment like "# TODO: implement
main loop (process tasks, handle shutdown)" and demonstrate using time.sleep or
a stop flag to avoid busy-waiting.
In `@ovos_plugin_manager/plugin_entry.py`:
- Around line 232-236: The GitHub URL check in plugin_entry.py is case-sensitive
and can miss URLs like "https://GitHub.com/..."; update the conditional that
inspects self.url (the branch that calls pip_install with a "git+" URL) to
perform a case-insensitive check (e.g., by lowercasing or casefolding self.url
before searching for "github") so that URLs with mixed case still match; keep
the existing guard that self.url is truthy and still call pip_install("git+" +
self.url) when matched.
In `@ovos_plugin_manager/templates/gui.py`:
- Around line 218-240: The class-level mapping _TEMPLATE_HANDLERS is currently
typed as Dict[str, str]; change its annotation to use typing.ClassVar (e.g.,
ClassVar[Dict[str, str]]) so type checkers know it’s a true class constant;
update the import to include ClassVar if missing and adjust the
_TEMPLATE_HANDLERS declaration accordingly in the gui.py file (referencing the
_TEMPLATE_HANDLERS symbol and the module-level typing imports).
In `@test/unittests/test_installation.py`:
- Line 2: Remove the unused import symbol `call` from the top-level import
statement so only actually used mocks are imported; update the line importing
from unittest.mock (where `patch, MagicMock, call` are listed) to drop `call`
(leaving `patch` and `MagicMock`) to eliminate the unused import warning.
In `@test/unittests/test_pipeline.py`:
- Line 4: Remove the unused import PluginConfigTypes from the import statement
in test/unittests/test_pipeline.py; keep only the used symbol PluginTypes in the
from ovos_plugin_manager.utils import ... line so the test module no longer
imports an unused name (locate the import that currently references
PluginConfigTypes alongside PluginTypes).
- Around line 11-49: Replace the nested-class lazy-import pattern by importing
or referencing the base classes directly and defining the impl classes at module
level: import PipelinePlugin and ConfidenceMatcherPipeline (or move those
imports into the match/match_high/match_medium/match_low methods) and declare
_ConcretePipeline._Impl and _ConcreteConfidenceMatcher._Impl as simple
subclasses of those bases (or assign _ConcretePipeline = _Impl and
_ConcreteConfidenceMatcher = _Impl) so you remove the outer wrapper classes
while preserving the same behavior; adjust references to _Impl,
_ConcretePipeline and _ConcreteConfidenceMatcher accordingly.
In `@test/unittests/test_transformers.py`:
- Around line 243-281: The tests in TestTransformerPluginUtils currently
hardcode four find_* tests while PLUGIN_TYPE_MAP contains six entries (including
"tts" and "intent"); update the tests to iterate PLUGIN_TYPE_MAP with a loop and
subTest so assertions and cases stay in sync: for each key, import the
corresponding finder function (e.g., find_utterance_transformer_plugins,
find_metadata_transformer_plugins, find_audio_transformer_plugins,
find_dialog_transformer_plugins, find_tts_transformer_plugins,
find_intent_transformer_plugins) inside the loop, call it while patching
ovos_plugin_manager.utils.find_plugins, and assert
mock_find.assert_called_once_with(the PluginTypes value from PLUGIN_TYPE_MAP).
Alternatively, if only the four finders should exist, remove the unused "tts"
and "intent" entries from PLUGIN_TYPE_MAP to keep the map accurate.
In `@test/unittests/test_tts.py`:
- Line 217: Remove the unused top-level import json from test_tts.py (the import
on the line that currently reads "import json") because json.load is being
patched directly via patch("json.load", ...) in the test; simply delete that
import to eliminate the unused-symbol warning and keep the test file clean.
In `@test/unittests/test_wakewords.py`:
- Line 1: Remove the unused top-level import "import json" from the test file;
the test "test_get_wws" already patches json.load via patch("json.load", ...) so
the module-level import is unnecessary—delete the "import json" line to clean up
the unused import.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1537acb8-6fe7-4b2c-bb8f-9a21c2595abd
📒 Files selected for processing (43)
docs/api/agents.mddocs/api/language.mddocs/api/microphone.mddocs/api/phal.mddocs/api/pipeline.mddocs/api/stt.mddocs/api/transformers.mddocs/api/tts.mddocs/api/vad.mddocs/api/wake-word.mddocs/configuration.mddocs/index.mddocs/installation.mddocs/plugin-types.mddocs/writing-plugins.mdovos_plugin_manager/exceptions.pyovos_plugin_manager/installation.pyovos_plugin_manager/plugin_entry.pyovos_plugin_manager/stt.pyovos_plugin_manager/templates/gui.pyovos_plugin_manager/templates/hotwords.pyovos_plugin_manager/templates/phal.pyovos_plugin_manager/templates/stt.pyovos_plugin_manager/templates/transformers.pyovos_plugin_manager/templates/tts.pyovos_plugin_manager/templates/vad.pyovos_plugin_manager/tts.pyovos_plugin_manager/utils/__init__.pyovos_plugin_manager/utils/config.pyovos_plugin_manager/wakewords.pytest/unittests/test_g2p.pytest/unittests/test_installation.pytest/unittests/test_language.pytest/unittests/test_phal.pytest/unittests/test_pipeline.pytest/unittests/test_plugin_entry.pytest/unittests/test_solver.pytest/unittests/test_stt.pytest/unittests/test_transformers.pytest/unittests/test_tts.pytest/unittests/test_utils.pytest/unittests/test_vad.pytest/unittests/test_wakewords.py
| | `TTS` | `opm.tts` | `TTS` | | ||
| | `WAKEWORD` | `opm.wake_word` | `HotWordEngine` | | ||
| | `WAKEWORD_VERIFIER` | `opm.wake_word.verifier` | `HotWordVerifier` | | ||
| | `VAD` | `opm.VAD` | `VADEngine` | |
Contributor
There was a problem hiding this comment.
Sync this table with the actual entry-point map.
This page currently uses both opm.VAD and opm.vad as the canonical VAD group, and it also omits ovos.plugin.audio2ipa even though DEPRECATED_ENTRYPOINTS still recognizes it in ovos_plugin_manager/utils/__init__.py. Since plugin authors will copy these strings verbatim, those mismatches can easily lead to undiscoverable plugins.
Also applies to: 83-106
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/plugin-types.md` at line 16, Update the table so its plugin group
strings exactly match the actual entry-point map constants: use the canonical
group name used in the entry-point map (replace inconsistent uses of
`opm.VAD`/`opm.vad` with the single canonical key), ensure the engine class
column uses `VADEngine`, and add the missing deprecated entry
`ovos.plugin.audio2ipa` (as listed in `DEPRECATED_ENTRYPOINTS` in
`ovos_plugin_manager/utils/__init__.py`); also audit and fix the same
inconsistencies on lines 83-106 so all table rows mirror the real entry-point
map.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ovos_plugin_manager/tts.py (1)
170-178:⚠️ Potential issue | 🟡 Minor
get_voicescan raiseFileNotFoundErrorif voices were never scanned.If
scan_voices()has never been called andscan=Falseis passed (the default),os.listdir(VOICES_FOLDER)at Line 175 will raiseFileNotFoundErrorbecause the directory doesn't exist. Consider handling missing directories gracefully:Suggested fix
if scan: scan_voices() voice_ids = {} for lang in get_tts_supported_langs(): VOICES_FOLDER = f"{xdg_data_home()}/OPM/voice_configs/{lang}" + if not os.path.isdir(VOICES_FOLDER): + continue for voice in os.listdir(VOICES_FOLDER): with open(f"{VOICES_FOLDER}/{voice}") as f: voice_ids[voice] = json.load(f) return voice_ids🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovos_plugin_manager/tts.py` around lines 170 - 178, The get_voices logic can raise FileNotFoundError when VOICES_FOLDER doesn't exist; update the loop that builds voice_ids (using get_tts_supported_langs(), VOICES_FOLDER, voice_ids) to skip missing directories instead of calling os.listdir on a non-existent path — e.g., check existence with os.path.isdir(VOICES_FOLDER) (or wrap os.listdir/ open in a try/except for FileNotFoundError) and continue if absent, and still call scan_voices() only when scan is True so behavior remains the same otherwise.ovos_plugin_manager/plugin_entry.py (1)
305-308:⚠️ Potential issue | 🔴 CriticalType mismatch:
pip_installexpectsList[str], notstr.The
pip_installfunction signature atovos_plugin_manager/installation.pyrequirespackages: List[str]. At lines 307 and 309, single strings are passed directly, which will be iterated character-by-character instead of treating them as package names.🐛 Proposed fix
if self.package_name: - return pip_install(self.package_name) + return pip_install([self.package_name]) if self.url and "github" in self.url: - return pip_install("git+" + self.url) + return pip_install(["git+" + self.url])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovos_plugin_manager/plugin_entry.py` around lines 305 - 308, The calls to pip_install are passing a single string which will be treated as an iterable of characters; change them to pass a list of package strings instead — replace pip_install(self.package_name) with pip_install([self.package_name]) and pip_install("git+" + self.url) with pip_install(["git+" + self.url]) so pip_install (from ovos_plugin_manager/installation.py) receives a List[str]; keep the existing conditional logic around package_name and url unchanged.
🧹 Nitpick comments (12)
test/unittests/test_pipeline.py (1)
4-4: Minor:PluginConfigTypesis imported but unused.This import can be removed if not needed.
-from ovos_plugin_manager.utils import PluginTypes, PluginConfigTypes +from ovos_plugin_manager.utils import PluginTypes🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_pipeline.py` at line 4, Remove the unused import PluginConfigTypes from the import statement that currently reads "from ovos_plugin_manager.utils import PluginTypes, PluginConfigTypes" in the test module; change it to only import PluginTypes (or remove the entire import if PluginTypes is also unused) and verify there are no remaining references to PluginConfigTypes elsewhere in the file (look for the symbol PluginConfigTypes and the import line to update).test/unittests/test_solver.py (1)
68-75: Register patch cleanup insetUpinstead of relying only ontearDown.If one
start()succeeds and a later setup step fails,tearDown()never runs and the earlier patch leaks into subsequent tests. Callingself.addCleanup(patcher.stop)immediately after each successfulstart()makes the isolation robust in both classes.Suggested pattern
self._det_patcher = patch( "ovos_plugin_manager.thirdparty.solvers.OVOSLangDetectionFactory.create", return_value=mock_detector) self._tx_patcher = patch( "ovos_plugin_manager.thirdparty.solvers.OVOSLangTranslationFactory.create", return_value=Mock()) - self._det_patcher.start() - self._tx_patcher.start() + self._det_patcher.start() + self.addCleanup(self._det_patcher.stop) + self._tx_patcher.start() + self.addCleanup(self._tx_patcher.stop)Apply the same pattern in
TestAutoTranslate.setUp.Also applies to: 83-84, 438-445, 450-455
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_solver.py` around lines 68 - 75, The setUp currently starts patches (self._det_patcher, self._tx_patcher) but doesn't register their cleanup immediately, which can leak patches if setUp fails; after each successful start() call (for the patchers created for OVOSLangDetectionFactory.create and OVOSLangTranslationFactory.create) call self.addCleanup(self._det_patcher.stop) and self.addCleanup(self._tx_patcher.stop) so the patchers are guaranteed to be stopped; apply the same pattern in TestAutoTranslate.setUp and for the other patchers mentioned around lines 83-84, 438-445, and 450-455.ovos_plugin_manager/templates/transformers.py (1)
210-220: Inconsistent indentation in docstring.Line 215 uses a tab character for indentation (
\tchunk (bytes)) while the rest of the codebase uses spaces. This may cause rendering issues in documentation tools.Suggested fix
def feed_speech_utterance(self, chunk: bytes) -> bytes: """ Process a complete speech utterance and return the (possibly modified) audio. Parameters: - chunk (bytes): Raw audio bytes of the complete speech utterance. + chunk (bytes): Raw audio bytes of the complete speech utterance. Returns: - audio (bytes): The processed audio bytes (modified or original). + bytes: The processed audio bytes (modified or original). """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovos_plugin_manager/templates/transformers.py` around lines 210 - 220, The docstring for feed_speech_utterance uses a tab character before the "chunk (bytes)" parameter which is inconsistent with the project's space-based indentation; update the docstring inside the feed_speech_utterance method to replace the tab with the same space indentation style used elsewhere so the parameter line aligns with the rest of the docstring and avoids rendering issues in documentation tools.test/unittests/test_transformers.py (2)
262-300: Test coverage incomplete for transformer plugin finders.
PLUGIN_TYPE_MAPincludesttsandintenttransformer types, but no tests exist forfind_tts_transformer_plugins()orfind_intent_transformer_plugins(). Consider adding tests for completeness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_transformers.py` around lines 262 - 300, Add two unit tests mirroring the existing transformer finder tests: patch ovos_plugin_manager.utils.find_plugins and call find_tts_transformer_plugins and find_intent_transformer_plugins respectively, then assert mock_find.assert_called_once_with(PluginTypes.TTS_TRANSFORMER) for the TTS test and mock_find.assert_called_once_with(PluginTypes.INTENT_TRANSFORMER) for the intent test; follow the same pattern and naming as test_find_utterance_transformers/test_find_dialog_transformers to keep consistency.
11-49: Unusual but functional pattern for scoped imports.The class-wrapper pattern to scope imports (
_ConcreteIntentTransformer._Impl) works but is unconventional. A simpler approach would be a module-level import with a direct subclass definition. That said, this doesn't affect correctness.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_transformers.py` around lines 11 - 49, The test uses an unusual nested wrapper to scope imports: replace the inner-class wrapper pattern for _ConcreteIntentTransformer and _ConcreteAudioLangDetector by importing the base classes at module scope (IntentTransformer, AudioLanguageDetector) and defining direct subclasses (e.g., class _ConcreteIntentTransformer(IntentTransformer): def transform(...): return intent and class _ConcreteAudioLangDetector(AudioLanguageDetector): def detect(...): return ("en-US", 0.99)); remove the outer wrapper classes and the _Impl indirection so tests reference the concrete classes directly.test/unittests/test_utils.py (1)
533-537: Exclusion comment could be more precise.
QUESTION_SOLVERis excluded because itsPluginConfigTypesvalue is"opm.solver.config"(not"opm.solver.question.config"), so thef"{plug_type.value}.config"pattern doesn't match. The current comment "legacy inconsistent naming" is accurate but could note this specific discrepancy for future maintainers.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_utils.py` around lines 533 - 537, Update the exclusion comment to explicitly call out the naming mismatch for QUESTION_SOLVER: mention that PluginTypes.QUESTION_SOLVER maps to PluginConfigTypes value "opm.solver.config" (not "opm.solver.question.config"), so the f"{plug_type.value}.config" pattern doesn't apply; keep the existing logic in the block that skips PluginTypes.PERSONA, PluginTypes.GUI_ADAPTER, and PluginTypes.QUESTION_SOLVER but replace the generic "legacy inconsistent naming" note with this precise explanation referencing PluginTypes, PluginConfigTypes, and QUESTION_SOLVER.test/unittests/test_installation.py (1)
101-108: Test PipException with the actual usage pattern.In
installation.py,PipExceptionis instantiated with three arguments:(pip_code, stdout, stderr). The current test only verifies single-string construction. Consider adding a test that matches actual usage:def test_three_arg_construction(self): exc = PipException(1, "stdout output", "stderr output") self.assertIsInstance(exc, RuntimeError) self.assertIn("1", str(exc))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_installation.py` around lines 101 - 108, Add a new unit test method on the TestPipException test case that constructs PipException using the actual three-argument signature used in installation.py (e.g., PipException(1, "stdout output", "stderr output")), then assert it is a RuntimeError and that the string representation contains the pip_code (e.g., "1"); place the new method (name it test_three_arg_construction) inside the TestPipException class alongside test_is_runtime_error and test_message, referencing the PipException symbol.ovos_plugin_manager/wakewords.py (1)
158-162: Missing error handling for non-existent directory.If
WW_FOLDERdoesn't exist,os.listdir(WW_FOLDER)will raiseFileNotFoundError. Consider wrapping with a check or try/except:♻️ Suggested defensive handling
for lang in get_ww_supported_langs(): WW_FOLDER = f"{xdg_data_home()}/OPM/ww_configs/{lang}" + if not os.path.isdir(WW_FOLDER): + continue for voice in os.listdir(WW_FOLDER): with open(f"{WW_FOLDER}/{voice}") as f: ww_ids[voice] = json.load(f)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovos_plugin_manager/wakewords.py` around lines 158 - 162, The loop over get_ww_supported_langs() assumes WW_FOLDER exists and will throw FileNotFoundError when calling os.listdir; update the code around WW_FOLDER (constructed with xdg_data_home()) to defensively skip or handle missing directories—either check os.path.isdir(WW_FOLDER) before os.listdir or wrap the os.listdir/open block in try/except FileNotFoundError and continue; ensure ww_ids is only updated when files are successfully opened/parsed (json.load) so malformed or missing folders don't break the whole operation.ovos_plugin_manager/templates/gui.py (1)
1-1: Unused import:List.
Listis imported fromtypingbut not used anywhere in the file.🧹 Suggested fix
-from typing import Optional, Dict, Any, List +from typing import Optional, Dict, Any🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovos_plugin_manager/templates/gui.py` at line 1, The import line in templates/gui.py includes an unused symbol "List" from typing; remove "List" from the typing import (i.e. change the import from "Optional, Dict, Any, List" to only the used names "Optional, Dict, Any") to eliminate the unused import and clean up the module.test/unittests/test_plugin_entry.py (1)
160-169: Unnecessary mock:load_pluginis not called byinstall().The test patches
load_plugin, butinstall()doesn't call it — it only usespackage_nameandurl. The test passes for the right reason (no install source), but the mock is misleading.🧹 Remove unnecessary patch
def test_install_returns_false_without_source(self): - """ - Verifies that install() fails when the plugin has no install source and cannot be loaded. - - Asserts that install() returns False for a plugin with only a name and no package or URL when the plugin loader reports the plugin is not available. - """ + """install() returns False when neither package_name nor url is available.""" p = self._make_plugin({"name": "some-plugin"}) - with patch("ovos_plugin_manager.plugin_entry.load_plugin", return_value=None): - result = p.install() + result = p.install() self.assertFalse(result)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_plugin_entry.py` around lines 160 - 169, The test test_install_returns_false_without_source patches load_plugin unnecessarily; remove the with patch("ovos_plugin_manager.plugin_entry.load_plugin", ...) block and call p.install() directly (keeping p = self._make_plugin({"name": "some-plugin"}) and the self.assertFalse(result)); ensure the test still verifies install() returns False when there is no package_name or url since install() does not call load_plugin.ovos_plugin_manager/plugin_entry.py (1)
54-61:from_nameonly checks 4 of 60+ plugin types.The factory method searches only STT, TTS, WAKEWORD, and AUDIO registries. Plugins registered under other types (GUI, PHAL, SKILL, PERSONA, etc.) won't have their
plugin_typeset correctly, even if installed.This may be intentional to keep the common case fast, but worth documenting.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovos_plugin_manager/plugin_entry.py` around lines 54 - 61, The from_name factory only checks four registries (find_stt_plugins, find_tts_plugins, find_wake_word_plugins, find_audio_service_plugins) so plugins of other PluginTypes never get their plugin_type set; modify from_name to iterate over all PluginTypes (or use a mapping of PluginTypes -> finder functions) and call the appropriate finder for each type, setting data["plugin_type"] when the name is found; alternatively implement a generic finder (e.g., find_plugins_for_type) and use that in from_name to cover all PluginTypes.test/unittests/test_vad.py (1)
30-47: Unusual but functional pattern for concrete test double.The nested class pattern for
_ConcreteVADworks, though a simpler approach would be a top-level class definition:♻️ Simpler alternative
+from ovos_plugin_manager.templates.vad import VADEngine + -class _ConcreteVAD: - from ovos_plugin_manager.templates.vad import VADEngine as _Base - - class _Impl(_Base): - def is_silence(self, chunk) -> bool: - ... - return chunk == b'\x00' * len(chunk) - - -_ConcreteVAD = _ConcreteVAD._Impl +class _ConcreteVAD(VADEngine): + def is_silence(self, chunk) -> bool: + """Return True if chunk is all zeros (silence).""" + return chunk == b'\x00' * len(chunk)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_vad.py` around lines 30 - 47, The nested test-double uses an inner class _Impl inside _ConcreteVAD which is unnecessary; replace it with a single top-level concrete class that subclasses ovos_plugin_manager.templates.vad.VADEngine (e.g., class ConcreteVAD(VADEngine):) and implement the is_silence(self, chunk) -> bool method (same semantics as current implementation), then set the test reference _ConcreteVAD = ConcreteVAD (or rename usages accordingly). This removes the indirection of _ConcreteVAD._Impl and makes the test double clearer while keeping the is_silence behavior unchanged.
🤖 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_plugin_manager/installation.py`:
- Around line 74-91: The pip_install signature now requires a List[str] which
breaks callers that pass a single string; modify pip_install to accept either a
string or list by updating its parameter handling in pip_install to detect if
packages is a str and, if so, wrap it into a single-element list (packages =
[packages]) before proceeding; ensure the type hint is adjusted to Union[str,
List[str]] (or keep List[str] but coerce at runtime), and keep all existing
behavior (constraints, locking, logging) unchanged so calls from plugin_entry
that pass self.package_name or "git+" + self.url work correctly.
In `@ovos_plugin_manager/templates/gui.py`:
- Around line 89-99: The docstring references an entry point group but there is
no loader support; add loader functions and export to match existing GUI plugin
patterns: export AbstractGUIPlugin from the loader module
(ovos_plugin_manager.gui.py) and implement PluginTypes.GUI_ADAPTER handling by
adding find_gui_adapter_plugins() and load_gui_adapter_plugin() functions that
mirror the behavior of the existing GUI plugin loaders (discover plugins by
entry point group, filter/validate, and import/load them), and ensure
AbstractGUIPlugin is exported from the loader module so consumers can import it
from ovos_plugin_manager.gui.
In `@ovos_plugin_manager/templates/stt.py`:
- Around line 235-251: The transcribe method's return type annotation is
incorrect: change the signature of transcribe(self, audio: Optional[AudioData] =
None, lang: Optional[str] = None) -> List[Tuple[str, float]] to ->
List[Tuple[Optional[str], float]] to match the docstring and possible return
value of (None, 1.0); also ensure Optional is imported from typing if not
already and leave the body calling self.execute(audio, lang) unchanged so
behavior remains the same.
In `@ovos_plugin_manager/templates/vad.py`:
- Around line 28-51: The __init__ method has mismatched indentation for its
docstring and body which causes an IndentationError; fix by aligning the
triple-quoted docstring and the following statements inside the __init__ method
block so that the docstring is directly under def __init__(...) and subsequent
assignments (self.config_core = Configuration(), self.config = config or {},
self.sample_rate resolution, and properties like padding_duration_ms,
frame_duration_ms, thresh, num_padding_frames) are indented one level inside the
method; ensure all lines belong to the same indentation level so the method
compiles correctly.
In `@ovos_plugin_manager/wakewords.py`:
- Around line 155-156: get_wws(scan=True) currently calls scan_wws() which
always raises NotImplementedError, making the scan parameter unusable; to fix,
wrap the scan_wws() call inside get_wws in a try/except that catches
NotImplementedError and emits a clear warning (e.g., warnings.warn or module
logger.warning) stating that scan is not implemented and returning the normal
non-scanned result, thereby preserving the scan parameter without crashing;
alternatively, if you prefer to remove unused API, drop the scan parameter from
get_wws and callers, but the safer minimal change is to add the try/except
around the scan_wws() call in get_wws.
In `@test/unittests/test_pipeline.py`:
- Around line 230-234: The test's patch target is wrong: update the `@patch`
decorator so it patches the name resolved inside the pipeline module rather than
utils; change the patch from "ovos_plugin_manager.utils.find_plugins" to
"ovos_plugin_manager.pipeline.find_plugins" so the mocked function used by
find_pipeline_plugins() (which imports find_plugins locally) is correctly
replaced; keep the rest of the test (mock_find and
assert_called_once_with(PluginTypes.PIPELINE)) unchanged.
In `@test/unittests/test_plugin_entry.py`:
- Around line 143-158: The tests mock pip_install but currently assert a string
argument; after fixing plugin_entry.py to call pip_install with a list, update
the unit tests (test_install_via_package_name and test_install_via_github_url)
to expect a list argument: assert_called_once_with([...]) instead of a string,
matching the new call signature in Plugin.install (or the install method that
invokes pip_install). Also verify pip_install is still mocked from
ovos_plugin_manager.plugin_entry to keep behavior isolated.
In `@test/unittests/test_solver.py`:
- Around line 447-448: The test currently uses a MagicMock on
self.solver.translate that reverses strings, and because the wrapped
auto_translate also reverses, the final assertion can't tell if translate was
called; update the test to either (A) assert that self.solver.translate was
called the expected number of times with expected arguments (use
assert_called_once_with / call_count checks) when invoking auto_translate, or
(B) replace the MagicMock side_effect with a distinct, non-overlapping
transformation (e.g., prepend or append a marker) so the final result clearly
indicates whether solver.translate ran; target the self.solver.translate mock
and the auto_translate invocation in the test to implement this change.
---
Outside diff comments:
In `@ovos_plugin_manager/plugin_entry.py`:
- Around line 305-308: The calls to pip_install are passing a single string
which will be treated as an iterable of characters; change them to pass a list
of package strings instead — replace pip_install(self.package_name) with
pip_install([self.package_name]) and pip_install("git+" + self.url) with
pip_install(["git+" + self.url]) so pip_install (from
ovos_plugin_manager/installation.py) receives a List[str]; keep the existing
conditional logic around package_name and url unchanged.
In `@ovos_plugin_manager/tts.py`:
- Around line 170-178: The get_voices logic can raise FileNotFoundError when
VOICES_FOLDER doesn't exist; update the loop that builds voice_ids (using
get_tts_supported_langs(), VOICES_FOLDER, voice_ids) to skip missing directories
instead of calling os.listdir on a non-existent path — e.g., check existence
with os.path.isdir(VOICES_FOLDER) (or wrap os.listdir/ open in a try/except for
FileNotFoundError) and continue if absent, and still call scan_voices() only
when scan is True so behavior remains the same otherwise.
---
Nitpick comments:
In `@ovos_plugin_manager/plugin_entry.py`:
- Around line 54-61: The from_name factory only checks four registries
(find_stt_plugins, find_tts_plugins, find_wake_word_plugins,
find_audio_service_plugins) so plugins of other PluginTypes never get their
plugin_type set; modify from_name to iterate over all PluginTypes (or use a
mapping of PluginTypes -> finder functions) and call the appropriate finder for
each type, setting data["plugin_type"] when the name is found; alternatively
implement a generic finder (e.g., find_plugins_for_type) and use that in
from_name to cover all PluginTypes.
In `@ovos_plugin_manager/templates/gui.py`:
- Line 1: The import line in templates/gui.py includes an unused symbol "List"
from typing; remove "List" from the typing import (i.e. change the import from
"Optional, Dict, Any, List" to only the used names "Optional, Dict, Any") to
eliminate the unused import and clean up the module.
In `@ovos_plugin_manager/templates/transformers.py`:
- Around line 210-220: The docstring for feed_speech_utterance uses a tab
character before the "chunk (bytes)" parameter which is inconsistent with the
project's space-based indentation; update the docstring inside the
feed_speech_utterance method to replace the tab with the same space indentation
style used elsewhere so the parameter line aligns with the rest of the docstring
and avoids rendering issues in documentation tools.
In `@ovos_plugin_manager/wakewords.py`:
- Around line 158-162: The loop over get_ww_supported_langs() assumes WW_FOLDER
exists and will throw FileNotFoundError when calling os.listdir; update the code
around WW_FOLDER (constructed with xdg_data_home()) to defensively skip or
handle missing directories—either check os.path.isdir(WW_FOLDER) before
os.listdir or wrap the os.listdir/open block in try/except FileNotFoundError and
continue; ensure ww_ids is only updated when files are successfully
opened/parsed (json.load) so malformed or missing folders don't break the whole
operation.
In `@test/unittests/test_installation.py`:
- Around line 101-108: Add a new unit test method on the TestPipException test
case that constructs PipException using the actual three-argument signature used
in installation.py (e.g., PipException(1, "stdout output", "stderr output")),
then assert it is a RuntimeError and that the string representation contains the
pip_code (e.g., "1"); place the new method (name it test_three_arg_construction)
inside the TestPipException class alongside test_is_runtime_error and
test_message, referencing the PipException symbol.
In `@test/unittests/test_pipeline.py`:
- Line 4: Remove the unused import PluginConfigTypes from the import statement
that currently reads "from ovos_plugin_manager.utils import PluginTypes,
PluginConfigTypes" in the test module; change it to only import PluginTypes (or
remove the entire import if PluginTypes is also unused) and verify there are no
remaining references to PluginConfigTypes elsewhere in the file (look for the
symbol PluginConfigTypes and the import line to update).
In `@test/unittests/test_plugin_entry.py`:
- Around line 160-169: The test test_install_returns_false_without_source
patches load_plugin unnecessarily; remove the with
patch("ovos_plugin_manager.plugin_entry.load_plugin", ...) block and call
p.install() directly (keeping p = self._make_plugin({"name": "some-plugin"}) and
the self.assertFalse(result)); ensure the test still verifies install() returns
False when there is no package_name or url since install() does not call
load_plugin.
In `@test/unittests/test_solver.py`:
- Around line 68-75: The setUp currently starts patches (self._det_patcher,
self._tx_patcher) but doesn't register their cleanup immediately, which can leak
patches if setUp fails; after each successful start() call (for the patchers
created for OVOSLangDetectionFactory.create and
OVOSLangTranslationFactory.create) call self.addCleanup(self._det_patcher.stop)
and self.addCleanup(self._tx_patcher.stop) so the patchers are guaranteed to be
stopped; apply the same pattern in TestAutoTranslate.setUp and for the other
patchers mentioned around lines 83-84, 438-445, and 450-455.
In `@test/unittests/test_transformers.py`:
- Around line 262-300: Add two unit tests mirroring the existing transformer
finder tests: patch ovos_plugin_manager.utils.find_plugins and call
find_tts_transformer_plugins and find_intent_transformer_plugins respectively,
then assert mock_find.assert_called_once_with(PluginTypes.TTS_TRANSFORMER) for
the TTS test and
mock_find.assert_called_once_with(PluginTypes.INTENT_TRANSFORMER) for the intent
test; follow the same pattern and naming as
test_find_utterance_transformers/test_find_dialog_transformers to keep
consistency.
- Around line 11-49: The test uses an unusual nested wrapper to scope imports:
replace the inner-class wrapper pattern for _ConcreteIntentTransformer and
_ConcreteAudioLangDetector by importing the base classes at module scope
(IntentTransformer, AudioLanguageDetector) and defining direct subclasses (e.g.,
class _ConcreteIntentTransformer(IntentTransformer): def transform(...): return
intent and class _ConcreteAudioLangDetector(AudioLanguageDetector): def
detect(...): return ("en-US", 0.99)); remove the outer wrapper classes and the
_Impl indirection so tests reference the concrete classes directly.
In `@test/unittests/test_utils.py`:
- Around line 533-537: Update the exclusion comment to explicitly call out the
naming mismatch for QUESTION_SOLVER: mention that PluginTypes.QUESTION_SOLVER
maps to PluginConfigTypes value "opm.solver.config" (not
"opm.solver.question.config"), so the f"{plug_type.value}.config" pattern
doesn't apply; keep the existing logic in the block that skips
PluginTypes.PERSONA, PluginTypes.GUI_ADAPTER, and PluginTypes.QUESTION_SOLVER
but replace the generic "legacy inconsistent naming" note with this precise
explanation referencing PluginTypes, PluginConfigTypes, and QUESTION_SOLVER.
In `@test/unittests/test_vad.py`:
- Around line 30-47: The nested test-double uses an inner class _Impl inside
_ConcreteVAD which is unnecessary; replace it with a single top-level concrete
class that subclasses ovos_plugin_manager.templates.vad.VADEngine (e.g., class
ConcreteVAD(VADEngine):) and implement the is_silence(self, chunk) -> bool
method (same semantics as current implementation), then set the test reference
_ConcreteVAD = ConcreteVAD (or rename usages accordingly). This removes the
indirection of _ConcreteVAD._Impl and makes the test double clearer while
keeping the is_silence behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b5e3d02a-ea32-4a17-ba2f-885f33d9adef
📒 Files selected for processing (20)
ovos_plugin_manager/installation.pyovos_plugin_manager/plugin_entry.pyovos_plugin_manager/stt.pyovos_plugin_manager/templates/gui.pyovos_plugin_manager/templates/phal.pyovos_plugin_manager/templates/stt.pyovos_plugin_manager/templates/transformers.pyovos_plugin_manager/templates/tts.pyovos_plugin_manager/templates/vad.pyovos_plugin_manager/tts.pyovos_plugin_manager/utils/config.pyovos_plugin_manager/wakewords.pytest/unittests/test_installation.pytest/unittests/test_pipeline.pytest/unittests/test_plugin_entry.pytest/unittests/test_solver.pytest/unittests/test_stt.pytest/unittests/test_transformers.pytest/unittests/test_utils.pytest/unittests/test_vad.py
🚧 Files skipped from review as they are similar to previous changes (2)
- ovos_plugin_manager/utils/config.py
- ovos_plugin_manager/stt.py
Comment on lines
+89
to
+99
| class AbstractGUIPlugin: | ||
| """Base class for GUI adapter plugins. | ||
|
|
||
| Each installed adapter receives every template event simultaneously, | ||
| allowing multiple display backends (Qt, browser, TUI, etc.) to operate | ||
| concurrently. | ||
|
|
||
| Subclass this and implement only the handlers you need; all handlers | ||
| default to no-ops so partial implementations are valid. | ||
|
|
||
| Entry point group: ``opm.gui_adapter`` |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if AbstractGUIPlugin is exported from ovos_plugin_manager/gui.py
rg -n "AbstractGUIPlugin" ovos_plugin_manager/gui.py
# Check if there's a separate gui_adapter loader
fd -t f "gui" ovos_plugin_manager/ --exec grep -l "gui_adapter\|AbstractGUIPlugin" {}Repository: OpenVoiceOS/ovos-plugin-manager
Length of output: 111
🏁 Script executed:
# Check what's actually exported from ovos_plugin_manager/gui.py
cat -n ovos_plugin_manager/gui.py
# Check entry point configuration in setup.py/pyproject.toml
fd -t f "(setup.py|pyproject.toml|setup.cfg)" . | head -5Repository: OpenVoiceOS/ovos-plugin-manager
Length of output: 3966
🏁 Script executed:
# Search for gui_adapter entry point definition
rg -n "gui_adapter\|opm.gui" . -A 2 -B 2 --max-count=10Repository: OpenVoiceOS/ovos-plugin-manager
Length of output: 57
🏁 Script executed:
# Check __init__.py files to see what's exported from the package
fd -t f "__init__.py" ovos_plugin_manager/ -x cat -n {}Repository: OpenVoiceOS/ovos-plugin-manager
Length of output: 21359
🏁 Script executed:
# Search for gui_adapter usage
rg -n "gui_adapter\|GUI_ADAPTER" --type py
# Check if there's a separate loader for gui_adapter
fd -t f "\.py" ovos_plugin_manager/ -x grep -l "gui_adapter\|load.*gui.*adapter" {}
# Check what's actually exported from templates/gui.py
grep -n "class\|def\|^from\|^import" ovos_plugin_manager/templates/gui.py | head -30Repository: OpenVoiceOS/ovos-plugin-manager
Length of output: 1998
🏁 Script executed:
# Check AbstractGUIPlugin usage/imports
rg -n "AbstractGUIPlugin" --type pyRepository: OpenVoiceOS/ovos-plugin-manager
Length of output: 139
🏁 Script executed:
# Check main package __init__.py
cat -n ovos_plugin_manager/__init__.py
# Verify AbstractGUIPlugin is not importable from main package
rg "AbstractGUIPlugin" ovos_plugin_manager/__init__.pyRepository: OpenVoiceOS/ovos-plugin-manager
Length of output: 201
Export AbstractGUIPlugin from the loader module and implement GUI_ADAPTER loader support.
The docstring claims AbstractGUIPlugin supports the opm.gui_adapter entry point, but there's no loader implementation for it. The GUI_ADAPTER entry point type is defined in PluginTypes but has no corresponding find_gui_adapter_plugins() or load_gui_adapter_plugin() functions. Either add full loader support matching the pattern used for GUI plugins (including export from ovos_plugin_manager/gui.py), or update the docstring to remove the entry point group reference if this functionality is not intended.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ovos_plugin_manager/templates/gui.py` around lines 89 - 99, The docstring
references an entry point group but there is no loader support; add loader
functions and export to match existing GUI plugin patterns: export
AbstractGUIPlugin from the loader module (ovos_plugin_manager.gui.py) and
implement PluginTypes.GUI_ADAPTER handling by adding find_gui_adapter_plugins()
and load_gui_adapter_plugin() functions that mirror the behavior of the existing
GUI plugin loaders (discover plugins by entry point group, filter/validate, and
import/load them), and ensure AbstractGUIPlugin is exported from the loader
module so consumers can import it from ovos_plugin_manager.gui.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
ovos_plugin_manager/templates/stt.py (1)
221-247:⚠️ Potential issue | 🟠 MajorAlign the streaming return types with the
STTcontract.Line 221 widens
execute()toOptional[str], but the abstractSTT.execute()still promisesstr. That flows into Line 247, wheretranscribe()can still return[(None, 1.0)]even though Line 236 is annotated asList[Tuple[str, float]]. IfNoneis intended here, the baseSTTAPI and this override should useOptionalconsistently; otherwise this path should be made total.Possible local fix for the visible mismatch
- def transcribe(self, audio: Optional[AudioData] = None, - lang: Optional[str] = None) -> List[Tuple[str, float]]: + def transcribe(self, audio: Optional[AudioData] = None, + lang: Optional[str] = None) -> List[Tuple[Optional[str], float]]:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovos_plugin_manager/templates/stt.py` around lines 221 - 247, The override of execute() returns Optional[str] while the abstract STT.execute() promises str, which lets transcribe() return (None, 1.0) despite its List[Tuple[str, float]] annotation; fix by making the contract consistent: either change this class's execute() to guarantee a str (ensure stream_stop() never returns None or replace None with an empty string and document that) or update the base STT.execute() signature and transcribe() annotation to use Optional[str] (and update transcribe() return type to List[Tuple[Optional[str], float]]). Update the type hints for execute and transcribe (and the STT base signature if you choose the Optional route) and ensure any callers/implementations handle the chosen non-total/optional behavior accordingly.ovos_plugin_manager/utils/config.py (1)
178-182:⚠️ Potential issue | 🟠 MajorGuard missing config entry points before normalizing.
load_plugin_configs()can still returnNone, so.items()here will abort language-config discovery for the whole plugin type when one plugin has no config entry point. Coerce the result to{}before normalizing; using the existingnormalize_language_keys=Truepath also removes the duplicated normalization logic.Suggested fix
- plug_configs = \ - load_plugin_configs(plug, - PluginConfigTypes(f"{plug_type.value}.config")) - plug_configs = {standardize_lang_tag(k): conf - for k, conf in plug_configs.items()} + plug_configs = load_plugin_configs( + plug, + PluginConfigTypes(f"{plug_type.value}.config"), + normalize_language_keys=True, + ) or {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovos_plugin_manager/utils/config.py` around lines 178 - 182, load_plugin_configs(plug, PluginConfigTypes(f"{plug_type.value}.config")) can return None, so guard against that by coercing its result to an empty dict before iterating; replace the manual normalization with the existing normalize_language_keys=True option to avoid duplicated logic: ensure plug_configs = load_plugin_configs(...) or {} and pass normalize_language_keys=True (and remove the dict comprehension using standardize_lang_tag) so plug_configs is always a dict and language keys are normalized in one place.ovos_plugin_manager/installation.py (2)
18-34:⚠️ Potential issue | 🟠 Major
max_resultsis still documented as a total cap, but recursion resets the budget.
search_pip(..., max_results=10)can still yield more than 10 packages because the recursive call on Line 68 uses the original limit instead of the remaining budget. Please threadremaining = max_results - yieldedinto the recursive call before documenting this as a global cap.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovos_plugin_manager/installation.py` around lines 18 - 34, The search_pip function's recursive pagination currently passes the original max_results to the recursive call, allowing more than the intended total; update search_pip so after yielding items from the current page you compute remaining = max_results - yielded (and return/stop if remaining <= 0) and pass that remaining value as the max_results argument into the recursive call (the recursion site in search_pip), ensuring the total yielded across pages never exceeds the original max_results.
75-96:⚠️ Potential issue | 🟠 Major
print_logs=Truestill breaks the documented error path.On the print-through path,
Popenis created without captured streams, but the failure branch still readsproc.stdout/proc.stderr. A failed install will raiseAttributeErrorinstead ofPipException, so theRaises:contract here is not true for that mode.Minimal fix
- if print_logs: - proc = Popen(pip_command) - else: - proc = Popen(pip_command, stdout=PIPE, stderr=PIPE) + proc = Popen( + pip_command, + stdout=None if print_logs else PIPE, + stderr=None if print_logs else PIPE + ) pip_code = proc.wait() if pip_code != 0: - stderr = proc.stderr.read().decode() + stdout = proc.stdout.read().decode() if proc.stdout else "" + stderr = proc.stderr.read().decode() if proc.stderr else "" raise PipException( - pip_code, proc.stdout.read().decode(), stderr + pip_code, stdout, stderr )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovos_plugin_manager/installation.py` around lines 75 - 96, The bug is that when pip_install creates the subprocess with print_logs=True it doesn't capture stdout/stderr but later still reads proc.stdout/proc.stderr on failure, causing AttributeError; fix pip_install by always creating the Popen with stdout=PIPE and stderr=PIPE (in the Popen call in pip_install), and then if print_logs is True stream the captured output to the terminal (e.g., print the bytes/text from proc.communicate() or while reading lines) while retaining the buffered output to include in the PipException on failure so the failure path consistently raises PipException with the process output.
🤖 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_plugin_manager/utils/config.py`:
- Around line 183-187: The prefix check uses raw keys so include_dialects misses
keys whose normalized tags don't share the same string prefix; replace the
startswith check with a comparison on normalized/macro forms: call
standardize_lang_tag(language, macro=True) for each key and test whether that
normalized tag startswith the macro variable (or equals it) before merging;
update the loop over plug_configs (referencing include_dialects,
standardize_lang_tag, macro, plug_configs, plugin_configs, plug) to perform
matching on standardized tags rather than the raw language string.
---
Duplicate comments:
In `@ovos_plugin_manager/installation.py`:
- Around line 18-34: The search_pip function's recursive pagination currently
passes the original max_results to the recursive call, allowing more than the
intended total; update search_pip so after yielding items from the current page
you compute remaining = max_results - yielded (and return/stop if remaining <=
0) and pass that remaining value as the max_results argument into the recursive
call (the recursion site in search_pip), ensuring the total yielded across pages
never exceeds the original max_results.
- Around line 75-96: The bug is that when pip_install creates the subprocess
with print_logs=True it doesn't capture stdout/stderr but later still reads
proc.stdout/proc.stderr on failure, causing AttributeError; fix pip_install by
always creating the Popen with stdout=PIPE and stderr=PIPE (in the Popen call in
pip_install), and then if print_logs is True stream the captured output to the
terminal (e.g., print the bytes/text from proc.communicate() or while reading
lines) while retaining the buffered output to include in the PipException on
failure so the failure path consistently raises PipException with the process
output.
In `@ovos_plugin_manager/templates/stt.py`:
- Around line 221-247: The override of execute() returns Optional[str] while the
abstract STT.execute() promises str, which lets transcribe() return (None, 1.0)
despite its List[Tuple[str, float]] annotation; fix by making the contract
consistent: either change this class's execute() to guarantee a str (ensure
stream_stop() never returns None or replace None with an empty string and
document that) or update the base STT.execute() signature and transcribe()
annotation to use Optional[str] (and update transcribe() return type to
List[Tuple[Optional[str], float]]). Update the type hints for execute and
transcribe (and the STT base signature if you choose the Optional route) and
ensure any callers/implementations handle the chosen non-total/optional behavior
accordingly.
In `@ovos_plugin_manager/utils/config.py`:
- Around line 178-182: load_plugin_configs(plug,
PluginConfigTypes(f"{plug_type.value}.config")) can return None, so guard
against that by coercing its result to an empty dict before iterating; replace
the manual normalization with the existing normalize_language_keys=True option
to avoid duplicated logic: ensure plug_configs = load_plugin_configs(...) or {}
and pass normalize_language_keys=True (and remove the dict comprehension using
standardize_lang_tag) so plug_configs is always a dict and language keys are
normalized in one place.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 656f74cd-050f-48a0-9efc-e04ff81afbd7
📒 Files selected for processing (4)
ovos_plugin_manager/installation.pyovos_plugin_manager/templates/stt.pyovos_plugin_manager/templates/vad.pyovos_plugin_manager/utils/config.py
🚧 Files skipped from review as they are similar to previous changes (1)
- ovos_plugin_manager/templates/vad.py
Replace legacy inline/individual workflow files with standard reusable callers from OpenVoiceOS/gh-automations@dev: - Removed: build_tests.yml, pipaudit.yml, license_tests.yml, conventional-label.yaml (old naming conventions / inline steps) - Added: build-tests.yml, pip_audit.yml, license_check.yml, conventional-label.yml (reusable callers, standardized naming) - Added: coverage.yml, lint.yml, release-preview.yml, repo-health.yml (previously missing, now added per OVOS workflow standards) All new workflows pass system_deps: 'swig libssl-dev' where package installation is needed. build-tests.yml covers Python 3.10–3.14. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Test fixes (CI failures):
- test_microphone.py: replace removed assertNotEquals with assertNotEqual (Python 3.12+)
- test_pipeline.py: fix mock patch target to ovos_plugin_manager.pipeline.find_plugins
- test_skills.py: fix mock patch target to ovos_plugin_manager.skills.find_plugins
- test_plugin_entry.py: update assertions to expect list args for pip_install calls
- test_solver.py: add assert_has_calls/call_count to test_auto_translate_decorator
Source fixes (CodeRabbit):
- installation.py: fix recursive budget passing, Union[str,List[str]] signature, guard proc.stdout/stderr
- plugin_entry.py: pass package names and git urls as lists to pip_install
- templates/tts.py: bare except -> except Exception
- templates/vad.py: soften extract_speech docstring (may return None)
- templates/stt.py: fix return type annotation to List[Tuple[Optional[str], float]]
- utils/__init__.py: add GUI_ADAPTER to PluginConfigTypes for symmetry
- utils/config.py: add 'or {}' guard on load_plugin_configs; fix dialect matching logic
- wakewords.py: remove unreachable return after raise; wrap scan_wws in try/except NotImplementedError
- gui.py: add find_gui_adapter_plugins() and load_gui_adapter_plugin() helpers
Docs fixes (CodeRabbit):
- docs/api/pipeline.md: fix example plugins (adapt/padatious, not converse/common-QA)
- docs/api/tts.md: correct OVOSTTSFactory.create() failure modes documentation
- docs/api/vad.md: fix AudioFrame.duration type from int to float
- docs/configuration.md: correct dialect priority semantics
- docs/plugin-types.md: add ovos.plugin.audio2ipa to deprecated entry points table
- docs/writing-plugins.md: fix cls->self in @Property examples for STT and TTS
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
AbstractSolver uses `internal_lang` (not `default_lang`) to set the solver's default language. Passing `default_lang='en'` silently fell into **kwargs and was ignored, causing the solver to pick up the system locale (e.g. pt-PT) instead of English. This made test_auto_translate_decorator fail on any machine whose locale differs from English. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- templates/tts.py: lstrip/rstrip used as substring removers (character-set bug) fixed with removeprefix/removesuffix; validate_ssml discarded the return value of format_speak_tags making all SSML wrapping a no-op - templates/vad.py: extract_speech annotated -> bytes but can return None; changed to -> Optional[bytes] - stt.py: bare assert for lang validation replaced with raise ValueError (assert is disabled under python -O flag) - utils/config.py: get_valid_plugin_configs used .append(list) instead of .extend(list), corrupting the config list structure; bare except fixed - tts.py: os.listdir() called without os.path.isdir() guard; crashes on first run before voice cache is populated - wakewords.py: same os.listdir() without isdir guard - templates/transformers.py: (list, dict) tuple-expression used as return annotation; fixed to Tuple[List[str], dict] and dict respectively - skills.py, ocp.py, installation.py, templates/solvers.py, templates/audio.py: bare except: → except Exception: - agents.py, embeddings.py, intent_transformers.py, triples.py, gui_adapter.py: Apache 2.0 headers, module docstrings, type(X) return annotations replaced with Optional[Type[X]], bare dict → Dict[str, Type[X]] Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Added 24 new test files and expanded 3 existing stubs, covering the highest-impact uncovered modules: New files: - test_embeddings_templates.py: EmbeddingsDB, TextEmbedder, ImageEmbedder, VoiceEmbedder, FaceEmbedder base class logic (was 28%) - test_tts_template_streaming.py: ConcatTTS and StreamingTTS threading paths - test_phal_template_extended.py: PHAL plugin base class coverage (was 57%) - test_thirdparty_sr.py: SpeechRecognition wrapper with mocked audio - test_media_templates.py: MediaBackend abstract base class methods - test_led_animations.py: all LED animation classes with mocked hardware - test_embeddings.py, test_embeddings_distance.py: distance metrics, vectors - test_agents.py, test_gui_module.py, test_gui_plugin_template.py, test_intent_transformers_triples_gui_adapter.py: plugin factory functions - test_audio_backend_template.py, test_g2p_template.py, test_coreference_legacy.py, test_tokenization_template.py, test_tts_template_extended.py, test_tts_cache.py, test_ocp_extended.py, test_sr_audiodata.py, test_thirdparty_solvers.py: gap-filling across template modules Expanded: - test_installation.py, test_pipeline.py, test_skills.py All test methods carry -> None annotations and docstrings per standards. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- AUDIT.md: complete file:line-cited findings — critical bugs (tts.py SSML no-op, vad.py None return, config.py list append, assert validation, bare excepts), performance issues (entry-point rescans, O(n) sets, scipy hot-path import), type annotation gaps, and code smells - SUGGESTIONS.md: architecture proposals (plugin discovery caching, distance() dispatch table, thirdparty/sr.py removal timeline, unify find_X/load_X boilerplate via factory) - FAQ.md: keyword-rich Q&A for common plugin manager questions - QUICK_FACTS.md: machine-readable package reference - MAINTENANCE_REPORT.md: AI-transparency log of all changes Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Use pytest.importorskip("numpy") so the test module is gracefully
skipped in CI environments that do not have numpy, instead of causing
a collection error that blocks all other tests.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
JarbasAl
changed the title
Remove AbstractGUIPlugin, gui_adapter.py, GUI_ADAPTER enum and related tests — these belong in a separate PR (feat/gui-plugin branch). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
I've completed the automated review of your changes. 📑I've aggregated the results of the automated checks for this PR below. 🏷️ Release PreviewI've checked the 'Internal Changes' section. ⚙️ Current:
✅ PR title follows conventional commit format. 🚀 Release Channel Compatibility Predicted next version:
📋 Repo HealthEnsuring the repository stays strong and healthy. 💪 ✅ All required files present. Latest Version: ✅ ⚖️ License CheckChecking the terms and conditions of your code. 📝 ✅ No license violations found (40 packages). License distribution: 10× MIT License, 6× Apache Software License, 6× MIT, 4× Apache-2.0, 2× BSD-3-Clause, 2× ISC License (ISCL), 2× PSF-2.0, 2× Python Software Foundation License, +6 more Full breakdown — 40 packages
Copyright (c) 2022 Phil Ewels Permission is hereby granted, free of charge, to any person obtaining a copy The above copyright notice and this permission notice shall be included in all THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR Policy: Apache 2.0 (universal donor). StrongCopyleft / NetworkCopyleft / WeakCopyleft / Other / Error categories fail. MPL allowed. 🔍 LintChecking if there's anything else we need to do. 📋 ❌ ruff: issues found — see job log 🔨 Build TestsChecking the plumbing of your data flows. 🚰 ✅ All versions pass
🔒 Security (pip-audit)Looking for any Trojan horses in the dependencies. 🐎 ✅ No known vulnerabilities found (59 packages scanned). 📊 CoverageEnsuring we've got all the bases covered! ⚾ Files below 80% coverage (44 files)
Full report: download the Keeping things running like clockwork 🕰️ |
GUI_ADAPTER was moved to the feat/gui-plugin branch; remove it from the exclusion list so the test does not fail on AttributeError. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
ovos_plugin_manager/templates/transformers.py (2)
120-133:⚠️ Potential issue | 🟡 Minor
@abc.abstractmethodhas no effect withoutabc.ABCinheritance.
IntentTransformerdoesn't inherit fromabc.ABC, so the@abc.abstractmethoddecorator ontransformis effectively a no-op. The class can still be instantiated directly.Either inherit from
ABCto enforce the abstract contract, or remove the decorator to match the pattern of other transformers in this file (which provide default implementations without marking them abstract).Option 1: Add ABC inheritance
-class IntentTransformer: +class IntentTransformer(abc.ABC):Option 2: Remove decorator to match other transformers
- `@abc.abstractmethod` def transform(self, intent: IntentHandlerMatch) -> IntentHandlerMatch:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovos_plugin_manager/templates/transformers.py` around lines 120 - 133, The IntentTransformer class currently decorates transform with `@abc.abstractmethod` but does not inherit from abc.ABC, so the abstract decorator is ineffective; either make IntentTransformer inherit from abc.ABC (e.g., class IntentTransformer(abc.ABC):) to enforce the abstract contract or remove the `@abc.abstractmethod` decorator from the transform method to keep the provided default implementation consistent with other transformers—update the class declaration (IntentTransformer) or remove the decorator on transform accordingly.
341-343:⚠️ Potential issue | 🟡 MinorSame
@abc.abstractmethodissue asIntentTransformer.
AudioLanguageDetectorinherits fromAudioTransformer, which doesn't inherit fromabc.ABC, so the@abc.abstractmethoddecorator has no effect. Theraise NotImplementedErrorprovides runtime enforcement, but the decorator is misleading.For consistency, either add
abc.ABCtoAudioTransformer's inheritance chain (which would affect all its subclasses) or remove the decorator and rely solely onNotImplementedError.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovos_plugin_manager/templates/transformers.py` around lines 341 - 343, The `@abc.abstractmethod` on AudioLanguageDetector.detect is ineffective because AudioTransformer (the parent) does not inherit from abc.ABC; fix by either (a) making AudioTransformer inherit from abc.ABC so abstractmethod behaves correctly for all subclasses (update the class definition of AudioTransformer to include abc.ABC), or (b) remove the `@abc.abstractmethod` decorator from AudioLanguageDetector.detect and keep the runtime NotImplementedError as the enforcement mechanism; pick one approach and apply it consistently (also review IntentTransformer for the same pattern to keep behavior uniform).ovos_plugin_manager/templates/vad.py (1)
11-17:⚠️ Potential issue | 🟡 Minor
AudioFrame.durationtype inconsistency with documentation.The
durationparameter is typed asintin the constructor (line 14), but the API documentation (docs/api/vad.mdline 67) documents it asfloatwith "Duration of the frame in seconds." For sub-second durations (e.g., 0.03s for 30ms frames), this should befloat.📝 Suggested fix
class AudioFrame: """Represents a "frame" of audio data.""" - def __init__(self, audio: bytes, timestamp: float, duration: int): + def __init__(self, audio: bytes, timestamp: float, duration: float): self.bytes = audio self.timestamp = timestamp self.duration = duration🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovos_plugin_manager/templates/vad.py` around lines 11 - 17, The constructor for AudioFrame declares duration as int but the API docs expect a float (seconds); update the type annotation and any related usage to float by changing the parameter and attribute type in the AudioFrame __init__ (AudioFrame.duration) to float and ensure callers/consumers treat duration as seconds (e.g., 0.03) rather than integer samples or milliseconds.
♻️ Duplicate comments (1)
ovos_plugin_manager/wakewords.py (1)
140-141:⚠️ Potential issue | 🟡 MinorRemove unused variable before raise.
The
ww_ids = {}assignment on line 140 is dead code sinceNotImplementedErroris raised immediately after. The static analysis (F841) correctly flags this.Suggested fix
def scan_wws() -> dict: """ Enumerate installed wake word plugins and cache their configurations. Returns: dict: Mapping from wake word id (str) to its configuration dict. Raises: NotImplementedError: Always — wake word metadata reporting is work in progress. """ - ww_ids = {} raise NotImplementedError("plugin wake word metadata reporting is WIP")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovos_plugin_manager/wakewords.py` around lines 140 - 141, Remove the dead assignment by deleting the unused variable ww_ids = {} that appears immediately before the NotImplementedError; locate the assignment in the wakewords handling code (the ww_ids variable) and simply remove that line so the raise NotImplementedError("plugin wake word metadata reporting is WIP") remains the only statement.
🟡 Minor comments (13)
test/unittests/test_g2p_template.py-19-19 (1)
19-19:⚠️ Potential issue | 🟡 MinorRemove unused imports.
MagicMockandpatchare imported but never used in this test file.Proposed fix
-from unittest.mock import MagicMock, patch🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_g2p_template.py` at line 19, Remove the unused imports MagicMock and patch from the import line in test_g2p_template.py; update the top-level import statement so it only imports what the test file actually uses (e.g., drop "MagicMock, patch" from the "from unittest.mock import ..." statement) to eliminate dead imports.test/unittests/test_agents.py-361-368 (1)
361-368:⚠️ Potential issue | 🟡 MinorRemove unused
resultvariable.The
resultvariable is assigned but never used. Either remove the assignment or add an assertion on the returned value for consistency with other tests.🔧 Proposed fix
`@patch`("ovos_plugin_manager.utils.find_plugins") def test_find_memory_plugins(self, mock_find: MagicMock) -> None: """find_memory_plugins calls find_plugins with AGENT_MEMORY.""" from ovos_plugin_manager.agents import find_memory_plugins mock_find.return_value = {"test": MagicMock()} - result = find_memory_plugins() + find_memory_plugins() mock_find.assert_called_once_with(PluginTypes.AGENT_MEMORY)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_agents.py` around lines 361 - 368, The test test_find_memory_plugins assigns an unused result variable; remove the assignment or assert the returned value for consistency: either call find_memory_plugins() without assigning to result and keep the existing mock_find.assert_called_once_with(PluginTypes.AGENT_MEMORY), or replace the unused assignment with an assertion like self.assertEqual(find_memory_plugins(), mock_find.return_value) (referencing the test function test_find_memory_plugins and the find_memory_plugins import and mock_find MagicMock).test/unittests/test_thirdparty_solvers.py-141-145 (1)
141-145:⚠️ Potential issue | 🟡 MinorThis doesn't cover the
None/falsy case claimed by the test name.
test_none_like_falsy_returns_empty_listis currently identical to the empty-string test. If the goal is to cover theif not textbranch more broadly, passNonehere or rename the test to match what it actually exercises.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_thirdparty_solvers.py` around lines 141 - 145, The test test_none_like_falsy_returns_empty_list currently calls self.solver.sentence_split("") which only covers the empty-string case; update it to call self.solver.sentence_split(None) so it actually exercises the None/falsy branch (or alternatively rename the test to reflect it only tests empty-string behavior); target the test method test_none_like_falsy_returns_empty_list and the sentence_split call to make this change.test/unittests/test_thirdparty_solvers.py-118-125 (1)
118-125:⚠️ Potential issue | 🟡 MinorThis cache test can pass even if caching is broken.
Because the patched factory always returns the same
MagicMock,first is secondstill holds whenOVOSLangTranslationFactory.create()is called twice. Please also assert the factory call count, or use distinct objects viaside_effect, so the test actually proves memoization.Suggested test hardening
def test_translator_cached_on_second_access(self): solver = _make_solver(enable_tx=False) mock_tx = MagicMock() - with patch("ovos_plugin_manager.thirdparty.solvers.OVOSLangTranslationFactory.create", - return_value=mock_tx): + with patch("ovos_plugin_manager.thirdparty.solvers.OVOSLangTranslationFactory.create", + side_effect=[mock_tx, MagicMock()]) as create_tx: first = solver.translator second = solver.translator self.assertIs(first, second) + create_tx.assert_called_once()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_thirdparty_solvers.py` around lines 118 - 125, The test test_translator_cached_on_second_access currently uses a patched OVOSLangTranslationFactory.create that always returns the same MagicMock so the identity assertion can pass even if caching is broken; update the test to either (a) assert OVOSLangTranslationFactory.create was called exactly once after accessing solver.translator twice, or (b) patch create with a side_effect that returns two distinct objects and then assert first is second to prove memoization; reference OVOSLangTranslationFactory.create and solver.translator when making the change.test/unittests/test_skills.py-130-137 (1)
130-137:⚠️ Potential issue | 🟡 MinorThis test doesn't actually exercise the permission fallback.
With
conf = {"skills": {}}, the function already picks the XDG path before the firstmakedirscall, so the "fallback" path is the same as the initial one. This still passes if theexcept PermissionErrorbranch is removed. Seed a non-XDG initial path, then assert the second call falls back to XDG.Suggested test adjustment
`@patch`("ovos_plugin_manager.skills.makedirs", side_effect=[PermissionError, None]) `@patch`("ovos_plugin_manager.skills.get_xdg_data_save_path", return_value="/xdg/save") def test_falls_back_to_xdg_on_permission_error(self, mock_xdg, mock_makedirs): from ovos_plugin_manager.skills import get_default_skills_directory - conf = {"skills": {}} + conf = {"skills": {"directory_override": "/opt/mycroft/skills"}} result = get_default_skills_directory(conf) - # Should fall back to xdg save path - self.assertIn("/xdg/save", result) + self.assertEqual(result, "/xdg/save/skills") + self.assertEqual( + mock_makedirs.call_args_list, + [ + unittest.mock.call("/opt/mycroft/skills", exist_ok=True), + unittest.mock.call("/xdg/save/skills", exist_ok=True), + ], + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_skills.py` around lines 130 - 137, The test currently sets conf = {"skills": {}} so get_default_skills_directory picks the XDG path before makedirs is exercised; change the test to provide a non-XDG initial directory (e.g. conf = {"skills": {"directory": "/non/xdg/path"}}) so the first makedirs call runs and raises PermissionError, forcing the function's except branch to call get_xdg_data_save_path; keep the patched makedirs side_effect=[PermissionError, None] and then assert the returned result contains "/xdg/save" to verify the fallback behavior in get_default_skills_directory.test/unittests/test_thirdparty_sr.py-203-209 (1)
203-209:⚠️ Potential issue | 🟡 MinorReplace blanket
try-except-passwith explicit skip logic.This pattern silently swallows any exception, including unexpected bugs. If
get_flac_converter()fails for reasons other than FLAC not being installed (e.g., a code regression), the test will pass silently, hiding the issue.💚 Suggested fix using unittest.skipIf or explicit handling
def test_returns_string(self) -> None: """get_flac_converter returns a string path.""" + import shutil + if shutil.which("flac") is None: + self.skipTest("FLAC converter not installed") - try: - path = get_flac_converter() - self.assertIsInstance(path, str) - except Exception: - pass # flac may not be installed in CI + path = get_flac_converter() + self.assertIsInstance(path, str)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_thirdparty_sr.py` around lines 203 - 209, The test test_returns_string currently swallows all exceptions from get_flac_converter; change it to explicitly skip when FLAC is not installed and let other errors fail: before calling get_flac_converter use a deterministic check (e.g., shutil.which('flac')) and call self.skipTest("flac not installed") if missing, or wrap the call and catch only the expected exception type (FileNotFoundError/OSError) and call self.skipTest with a clear message; do not use a blanket except/pass so unexpected exceptions from get_flac_converter surface as test failures.ovos_plugin_manager/ocp.py-68-72 (1)
68-72:⚠️ Potential issue | 🟡 MinorInclude the failure reason in this log.
Right now a broken plugin only produces
Failed to load <name>, which makes loader regressions much harder to diagnose in the field. Please log the exception message here as well.Suggested change
- except Exception: - LOG.error(f"Failed to load {plugin}") + except Exception as e: + LOG.error(f"Failed to load {plugin}: {e}") continue🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovos_plugin_manager/ocp.py` around lines 68 - 72, The error log in the plugin loading block (where self.extractors[plugin] = clazz() and LOG.info is called) currently swallows the exception; update the except Exception handler to capture the exception (e) and include its details in the log—preferably using LOG.exception or LOG.error with the exception message and context (plugin name) so failures to load a plugin (plugin, clazz) show the error/stacktrace for easier diagnosis.test/unittests/test_led_animations.py-108-114 (1)
108-114:⚠️ Potential issue | 🟡 MinorMake the timeout tests deterministic.
These cases rely on a 1ms real timeout and only assert that some LED calls happened, so they can be flaky and still miss a regression in the actual timeout path. Prefer stubbing the delay/clock used by the animation and then asserting timeout-specific state or cleanup.
Also applies to: 165-171, 264-270, 357-364, 399-406
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_led_animations.py` around lines 108 - 114, The timeout test (test_start_timeout) currently relies on a real 1ms sleep and only checks that some LED calls occurred; replace this with a deterministic stub of the animation's timing mechanism: mock or inject the clock/delay used by BreatheLedAnimation (e.g., override anim.step_delay loop or inject a fake timer) so you can advance time deterministically, call anim.start(timeout=...) and then assert timeout-specific behavior/cleanup (for example that anim.is_running is False, the timeout callback ran, or mock_leds.fill was called the expected final time) instead of relying on wall-clock sleep; apply the same pattern to the other timeout tests referenced (lines ~165, ~264, ~357, ~399) to make them deterministic.test/unittests/test_led_animations.py-116-123 (1)
116-123:⚠️ Potential issue | 🟡 MinorAssert the behavior this test is naming.
test_start_clears_stoppingcurrently has no assertion, so it will pass even ifstart()never clears the event.Proposed fix
def test_start_clears_stopping(self) -> None: """start() clears stopping event before running.""" mock_leds = _make_mock_leds() anim = BreatheLedAnimation(mock_leds, Color.GREEN) anim.stopping.set() anim.step_delay = 0 - anim.start(timeout=0.001) - # stopping was cleared and then set again by timeout branch + with patch.object(anim.stopping, "clear", wraps=anim.stopping.clear) as clear: + anim.start(timeout=0.001) + clear.assert_called_once()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_led_animations.py` around lines 116 - 123, The test test_start_clears_stopping should assert that BreatheLedAnimation.start clears the stopping event before running: after creating anim = BreatheLedAnimation(mock_leds, Color.GREEN) and setting anim.stopping.set(), call anim.start(timeout=0.001) and then assert that anim.stopping was cleared at the moment start began (e.g., assert not anim.stopping.is_set() immediately after start returns or by observing state changes if start sets it again on timeout); update the test to include that assert referencing anim.stopping and the start method so the test fails if start() does not clear the event.test/unittests/test_audio_backend_template.py-233-236 (1)
233-236:⚠️ Potential issue | 🟡 MinorThis test never forces the “no current message” branch.
Because
dig_for_message()is not stubbed, this passes whether_format_msgcreates a newMessageor forwards an ambient one. Patchovos_plugin_manager.templates.audio.dig_for_messageto returnNoneso the named behavior is actually under test.Suggested fix
- def test_format_msg_no_bus_msg(self) -> None: - """_format_msg creates a new Message when no current message.""" - msg = self.backend._format_msg("test.message", {"key": "val"}) + `@patch`("ovos_plugin_manager.templates.audio.dig_for_message", return_value=None) + def test_format_msg_no_bus_msg(self, _dig_for_message) -> None: + """_format_msg creates a new Message when no current message.""" + msg = self.backend._format_msg("test.message", {"key": "val"}) self.assertEqual(msg.msg_type, "test.message") + self.assertEqual(msg.data, {"key": "val"})🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_audio_backend_template.py` around lines 233 - 236, The test test_format_msg_no_bus_msg currently doesn't force the “no current message” branch because dig_for_message is not stubbed; update the test to patch ovos_plugin_manager.templates.audio.dig_for_message to return None before calling self.backend._format_msg so the code path that creates a new Message is executed; use your test framework's patch/monkeypatch (e.g., unittest.mock.patch or pytest monkeypatch) to stub dig_for_message to None, then call _format_msg and assert msg.msg_type == "test.message".docs/writing-plugins.md-54-60 (1)
54-60:⚠️ Potential issue | 🟡 MinorDocumentation example accesses
@propertyon the class instead of an instance.Line 56 calls
MyTTSPlugin.available_languageson the class, butavailable_languagesis defined as an instance@property(withself). This will raiseTypeErrorat runtime because properties require an instance.📝 Suggested fix
class MyTTSValidator(TTSValidator): def validate_lang(self): - assert self.tts.lang in MyTTSPlugin.available_languages + assert self.tts.lang in self.tts.available_languages def get_tts_class(self): return MyTTSPlugin🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/writing-plugins.md` around lines 54 - 60, The example incorrectly accesses the instance property MyTTSPlugin.available_languages from the class in MyTTSValidator.validate_lang; change the access to use the plugin instance (e.g., self.tts.available_languages) or make available_languages a `@classmethod/`@staticmethod if it must be accessed on the class—update the validate_lang method (MyTTSValidator.validate_lang) to reference the instance property instead of the class property to avoid the TypeError.ovos_plugin_manager/stt.py-22-33 (1)
22-33:⚠️ Potential issue | 🟡 MinorReturn type should be
Optional[Type[STT]]to match actual behavior.The underlying
load_pluginfunction can returnNonewhen the plugin is not found (as documented inAUDIT.mdline 89-90 and seen inintent_transformers.pyline 30). The return type annotation should reflect this, and the docstring should mention theNonecase.📝 Suggested fix
-def load_stt_plugin(module_name: str) -> Type[STT]: +def load_stt_plugin(module_name: str) -> Optional[Type[STT]]: """ Return the uninstantiated STT plugin class for the given plugin entrypoint name. Parameters: module_name (str): Plugin entrypoint name to load. Returns: - Type[STT]: The STT plugin class corresponding to module_name. + Type[STT] or None: The STT plugin class, or None if not found. """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovos_plugin_manager/stt.py` around lines 22 - 33, Change the return annotation and docs of load_stt_plugin to reflect that load_plugin may return None: update the function signature return type from Type[STT] to Optional[Type[STT]] and modify the docstring for load_stt_plugin to state that it may return None if the plugin entrypoint is not found (matching load_plugin behavior). Ensure references to PluginTypes.STT and the call to load_plugin(module_name, PluginTypes.STT) remain unchanged so the function logic is intact.test/unittests/test_installation.py-150-156 (1)
150-156:⚠️ Potential issue | 🟡 MinorRemove unnecessary f-string prefixes.
Lines 150, 153, and 155 use f-strings but contain no placeholders. Use regular strings instead.
Suggested fix
return ( - f'<span class="package-snippet__name">DUMMY</span>' # split[0] is ignored + '<span class="package-snippet__name">DUMMY</span>' # split[0] is ignored + names_html - + '<span class="package-snippet__name">DUMMY</span>' # last split ignored - + f'<p class="package-snippet__description">DUMMY</p>' + + '<span class="package-snippet__name">DUMMY</span>' # last split ignored + + '<p class="package-snippet__description">DUMMY</p>' + descs_html - + f'<p class="package-snippet__description">DUMMY</p>' + + '<p class="package-snippet__description">DUMMY</p>' + page_html )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_installation.py` around lines 150 - 156, The test builds an HTML fragment using unnecessary f-strings for static literals; replace the three f-string literals that wrap the DUMMY spans and paragraphs (the leading f'' strings surrounding '<span class="package-snippet__name">DUMMY</span>' and '<p class="package-snippet__description">DUMMY</p>') with plain string literals so they are regular quoted strings concatenated with names_html, descs_html, and page_html (look for the concatenation near the names_html, descs_html, and page_html variables in test_installation.py).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/build-tests.yml:
- Line 13: CI tests are missing Python 3.9 while pyproject.toml allows >=3.9;
update the GitHub Actions matrix by adding "3.9" to the python_versions list in
the workflow (the python_versions entry in .github/workflows/build-tests.yml) so
the matrix becomes ['3.9','3.10','3.11','3.12','3.13','3.14'], or alternatively
change the requires-python field in pyproject.toml to ">=3.10" to match the
current matrix—pick one approach and make the corresponding edit to either the
python_versions key or the requires-python value so declared support and CI
tests stay in sync.
In @.github/workflows/downstream.yml:
- Around line 12-13: Replace the mutable branch ref used when calling the
reusable workflow with an immutable commit SHA so the job cannot be altered to
access inherited secrets; specifically change the reusable workflow reference in
the uses entry (currently
"OpenVoiceOS/gh-automations/.github/workflows/downstream-check.yml@dev") to use
a full commit SHA (e.g.,
"OpenVoiceOS/gh-automations/.github/workflows/downstream-check.yml@<full-commit-sha>")
before keeping "secrets: inherit".
In @.github/workflows/pip_audit.yml:
- Around line 10-11: The workflow references a mutable reusable workflow via
"uses: OpenVoiceOS/gh-automations/.github/workflows/pip-audit.yml@dev" and also
uses "secrets: inherit"; replace the mutable ref with an immutable ref (a
specific commit SHA or release tag) in the uses line and, if you want to avoid
external CI changes without a PR here, stop inheriting external secrets by
removing or replacing "secrets: inherit" with explicitly listed secrets or
repository secrets—apply the same change consistently across all other workflows
that use the `@dev` ref.
In @.github/workflows/publish_stable.yml:
- Around line 10-11: The workflow currently uses a mutable ref for the reusable
workflow (uses:
OpenVoiceOS/gh-automations/.github/workflows/publish-stable.yml@dev) while also
inheriting secrets (secrets: inherit); replace the mutable `@dev` ref with a
specific reviewed commit SHA (or an immutable release tag) for the reusable
workflow reference to ensure the inherited secrets are only passed to a pinned,
auditable revision.
In @.github/workflows/release_workflow.yml:
- Line 11: The current workflow condition "if: github.event.pull_request.merged
== true || github.event_name == 'workflow_dispatch'" doesn't respect the
ignore-for-release label; update the if expression so merged-PR triggers only
when the PR does NOT have a label named "ignore-for-release". Specifically,
modify the existing GitHub Actions conditional that references
github.event.pull_request.merged (the if line) to include a negated contains
check against github.event.pull_request.labels.*.name (e.g., require merged ==
true AND NOT contains(..., 'ignore-for-release')), while leaving
workflow_dispatch triggers unchanged.
- Line 12: The reusable workflow reference currently points to a mutable ref
"OpenVoiceOS/gh-automations/.github/workflows/publish-alpha.yml@dev"; replace
the branch ref with an immutable commit SHA from the upstream repo (i.e., change
the "@dev" suffix to the specific commit SHA for the desired publish-alpha.yml
revision). Locate the uses declaration in .github/workflows/release_workflow.yml
(the line with uses:
OpenVoiceOS/gh-automations/.github/workflows/publish-alpha.yml@dev), fetch the
correct commit SHA from the OpenVoiceOS/gh-automations repository (via git
ls-remote or the commit page), and update the file to use that SHA so the
reusable workflow is pinned to an immutable revision.
In `@pyproject.toml`:
- Around line 13-25: Add a [project.optional-dependencies] table to the
pyproject.toml and define a "test" extra so pip install package[test] succeeds;
specifically, next to the existing dependencies list (the dependencies = [...]
block) add a new section beginning with [project.optional-dependencies] and
include a "test" key with an array of the CI/test dependencies used by your
workflow (e.g., pytest, pytest-cov, tox or any test-related packages your CI
expects) so that install_extras: 'test' resolves.
In `@test/unittests/test_media_templates.py`:
- Around line 211-224: The tests currently only check side effects like
backend._now_playing and backend.meta but must also assert that the message/bus
emitter receives the correct QUEUED_* and PLAYING_* state events; update the
tests for test_load_track, test_load_track_with_metadata, test_ocp_start (and
the other ranges noted) to patch/mock the emitter used by the media backend (the
same emitter the base class uses) and add assertions that a state message with
the expected QUEUED_* topic is emitted after load_track and that a PLAYING_*
message is emitted when ocp_start is called (and analogous checks for
ocp_pause); locate calls to load_track, ocp_start, ocp_pause and the backend
instance in these tests and replace or wrap the emitter to capture published
messages, then assert on the captured messages' topic/payload along with the
existing assertions on _now_playing/meta.
- Around line 35-180: The test backends (_ConcreteMediaBackend,
_ConcreteAudioBackend, _ConcreteVideoBackend, _ConcreteWebBackend) use pure
no-op lifecycle/seek hooks and always return position 0, which hides
regressions; change these classes to record calls and simulate realistic track
position changes: implement play/stop/pause/resume/lower_volume/restore_volume
to append events to an internal list (e.g., self.calls) and update a mutable
position state, implement set_track_position to update that state, and make
get_track_position/get_track_length return the simulated values so tests can
assert that ocp_start -> play(), shutdown -> stop(), and seek_* compute offsets
correctly by inspecting the recorded calls and positions.
In `@test/unittests/test_ocp_extended.py`:
- Around line 103-128: The tests for StreamHandler.extract_stream are too weak
because they only assert presence of "uri" (always present); update the
assertions to verify the specific transformed URIs returned by the mocked
extractor (e.g. expect "https://real-stream.com/v.mp4" for the SEI extractor
case and "https://stream.com/audio.m3u8" for the URL extractor case) and assert
the extractor interactions: confirm mock_extractor.extract_stream was called
once with the original URI and mock_cls (or other extractor mocks) were/not
called as appropriate to prove the SEI vs URL dispatch path was taken.
---
Outside diff comments:
In `@ovos_plugin_manager/templates/transformers.py`:
- Around line 120-133: The IntentTransformer class currently decorates transform
with `@abc.abstractmethod` but does not inherit from abc.ABC, so the abstract
decorator is ineffective; either make IntentTransformer inherit from abc.ABC
(e.g., class IntentTransformer(abc.ABC):) to enforce the abstract contract or
remove the `@abc.abstractmethod` decorator from the transform method to keep the
provided default implementation consistent with other transformers—update the
class declaration (IntentTransformer) or remove the decorator on transform
accordingly.
- Around line 341-343: The `@abc.abstractmethod` on AudioLanguageDetector.detect
is ineffective because AudioTransformer (the parent) does not inherit from
abc.ABC; fix by either (a) making AudioTransformer inherit from abc.ABC so
abstractmethod behaves correctly for all subclasses (update the class definition
of AudioTransformer to include abc.ABC), or (b) remove the `@abc.abstractmethod`
decorator from AudioLanguageDetector.detect and keep the runtime
NotImplementedError as the enforcement mechanism; pick one approach and apply it
consistently (also review IntentTransformer for the same pattern to keep
behavior uniform).
In `@ovos_plugin_manager/templates/vad.py`:
- Around line 11-17: The constructor for AudioFrame declares duration as int but
the API docs expect a float (seconds); update the type annotation and any
related usage to float by changing the parameter and attribute type in the
AudioFrame __init__ (AudioFrame.duration) to float and ensure callers/consumers
treat duration as seconds (e.g., 0.03) rather than integer samples or
milliseconds.
---
Minor comments:
In `@docs/writing-plugins.md`:
- Around line 54-60: The example incorrectly accesses the instance property
MyTTSPlugin.available_languages from the class in MyTTSValidator.validate_lang;
change the access to use the plugin instance (e.g.,
self.tts.available_languages) or make available_languages a
`@classmethod/`@staticmethod if it must be accessed on the class—update the
validate_lang method (MyTTSValidator.validate_lang) to reference the instance
property instead of the class property to avoid the TypeError.
In `@ovos_plugin_manager/ocp.py`:
- Around line 68-72: The error log in the plugin loading block (where
self.extractors[plugin] = clazz() and LOG.info is called) currently swallows the
exception; update the except Exception handler to capture the exception (e) and
include its details in the log—preferably using LOG.exception or LOG.error with
the exception message and context (plugin name) so failures to load a plugin
(plugin, clazz) show the error/stacktrace for easier diagnosis.
In `@ovos_plugin_manager/stt.py`:
- Around line 22-33: Change the return annotation and docs of load_stt_plugin to
reflect that load_plugin may return None: update the function signature return
type from Type[STT] to Optional[Type[STT]] and modify the docstring for
load_stt_plugin to state that it may return None if the plugin entrypoint is not
found (matching load_plugin behavior). Ensure references to PluginTypes.STT and
the call to load_plugin(module_name, PluginTypes.STT) remain unchanged so the
function logic is intact.
In `@test/unittests/test_agents.py`:
- Around line 361-368: The test test_find_memory_plugins assigns an unused
result variable; remove the assignment or assert the returned value for
consistency: either call find_memory_plugins() without assigning to result and
keep the existing mock_find.assert_called_once_with(PluginTypes.AGENT_MEMORY),
or replace the unused assignment with an assertion like
self.assertEqual(find_memory_plugins(), mock_find.return_value) (referencing the
test function test_find_memory_plugins and the find_memory_plugins import and
mock_find MagicMock).
In `@test/unittests/test_audio_backend_template.py`:
- Around line 233-236: The test test_format_msg_no_bus_msg currently doesn't
force the “no current message” branch because dig_for_message is not stubbed;
update the test to patch ovos_plugin_manager.templates.audio.dig_for_message to
return None before calling self.backend._format_msg so the code path that
creates a new Message is executed; use your test framework's patch/monkeypatch
(e.g., unittest.mock.patch or pytest monkeypatch) to stub dig_for_message to
None, then call _format_msg and assert msg.msg_type == "test.message".
In `@test/unittests/test_g2p_template.py`:
- Line 19: Remove the unused imports MagicMock and patch from the import line in
test_g2p_template.py; update the top-level import statement so it only imports
what the test file actually uses (e.g., drop "MagicMock, patch" from the "from
unittest.mock import ..." statement) to eliminate dead imports.
In `@test/unittests/test_installation.py`:
- Around line 150-156: The test builds an HTML fragment using unnecessary
f-strings for static literals; replace the three f-string literals that wrap the
DUMMY spans and paragraphs (the leading f'' strings surrounding '<span
class="package-snippet__name">DUMMY</span>' and '<p
class="package-snippet__description">DUMMY</p>') with plain string literals so
they are regular quoted strings concatenated with names_html, descs_html, and
page_html (look for the concatenation near the names_html, descs_html, and
page_html variables in test_installation.py).
In `@test/unittests/test_led_animations.py`:
- Around line 108-114: The timeout test (test_start_timeout) currently relies on
a real 1ms sleep and only checks that some LED calls occurred; replace this with
a deterministic stub of the animation's timing mechanism: mock or inject the
clock/delay used by BreatheLedAnimation (e.g., override anim.step_delay loop or
inject a fake timer) so you can advance time deterministically, call
anim.start(timeout=...) and then assert timeout-specific behavior/cleanup (for
example that anim.is_running is False, the timeout callback ran, or
mock_leds.fill was called the expected final time) instead of relying on
wall-clock sleep; apply the same pattern to the other timeout tests referenced
(lines ~165, ~264, ~357, ~399) to make them deterministic.
- Around line 116-123: The test test_start_clears_stopping should assert that
BreatheLedAnimation.start clears the stopping event before running: after
creating anim = BreatheLedAnimation(mock_leds, Color.GREEN) and setting
anim.stopping.set(), call anim.start(timeout=0.001) and then assert that
anim.stopping was cleared at the moment start began (e.g., assert not
anim.stopping.is_set() immediately after start returns or by observing state
changes if start sets it again on timeout); update the test to include that
assert referencing anim.stopping and the start method so the test fails if
start() does not clear the event.
In `@test/unittests/test_skills.py`:
- Around line 130-137: The test currently sets conf = {"skills": {}} so
get_default_skills_directory picks the XDG path before makedirs is exercised;
change the test to provide a non-XDG initial directory (e.g. conf = {"skills":
{"directory": "/non/xdg/path"}}) so the first makedirs call runs and raises
PermissionError, forcing the function's except branch to call
get_xdg_data_save_path; keep the patched makedirs side_effect=[PermissionError,
None] and then assert the returned result contains "/xdg/save" to verify the
fallback behavior in get_default_skills_directory.
In `@test/unittests/test_thirdparty_solvers.py`:
- Around line 141-145: The test test_none_like_falsy_returns_empty_list
currently calls self.solver.sentence_split("") which only covers the
empty-string case; update it to call self.solver.sentence_split(None) so it
actually exercises the None/falsy branch (or alternatively rename the test to
reflect it only tests empty-string behavior); target the test method
test_none_like_falsy_returns_empty_list and the sentence_split call to make this
change.
- Around line 118-125: The test test_translator_cached_on_second_access
currently uses a patched OVOSLangTranslationFactory.create that always returns
the same MagicMock so the identity assertion can pass even if caching is broken;
update the test to either (a) assert OVOSLangTranslationFactory.create was
called exactly once after accessing solver.translator twice, or (b) patch create
with a side_effect that returns two distinct objects and then assert first is
second to prove memoization; reference OVOSLangTranslationFactory.create and
solver.translator when making the change.
In `@test/unittests/test_thirdparty_sr.py`:
- Around line 203-209: The test test_returns_string currently swallows all
exceptions from get_flac_converter; change it to explicitly skip when FLAC is
not installed and let other errors fail: before calling get_flac_converter use a
deterministic check (e.g., shutil.which('flac')) and call self.skipTest("flac
not installed") if missing, or wrap the call and catch only the expected
exception type (FileNotFoundError/OSError) and call self.skipTest with a clear
message; do not use a blanket except/pass so unexpected exceptions from
get_flac_converter surface as test failures.
---
Duplicate comments:
In `@ovos_plugin_manager/wakewords.py`:
- Around line 140-141: Remove the dead assignment by deleting the unused
variable ww_ids = {} that appears immediately before the NotImplementedError;
locate the assignment in the wakewords handling code (the ww_ids variable) and
simply remove that line so the raise NotImplementedError("plugin wake word
metadata reporting is WIP") remains the only statement.
---
Nitpick comments:
In @.github/workflows/lint.yml:
- Around line 1-12: The reusable workflow reference uses the floating ref "@dev"
which can lead to unexpected CI behavior when that branch changes; update the
uses line in the workflow (the job using
OpenVoiceOS/gh-automations/.github/workflows/lint.yml@dev) to pin a stable ref
such as a specific commit SHA or a tagged release (e.g., replace "@dev" with
"@<commit-sha>" or "@vX.Y.Z"); retrieve the commit SHA from the upstream repo
and update the uses value, or create/use a documented tag, ensuring secrets:
inherit remains if desired.
In `@AUDIT.md`:
- Around line 1-226: Summary: The audit documents many critical bugs and
technical debt; we should convert the Critical items into tracked GitHub issues.
Action: create individual GitHub issues for each Critical entry (use the
file+symbol and short title), e.g. "Dead log branch in find_plugins
(utils.__init__.py: entrypoints insertion check)", "get_valid_plugin_configs
appends list instead of extending (utils/config.py:get_valid_plugin_configs)",
"replace assert guard in get_stt_config (stt.py:get_stt_config)", "check
VOICES_FOLDER exists before os.listdir in get_voices (tts.py:get_voices)",
"check WW_FOLDER exists before os.listdir in get_wws (wakewords.py:get_wws)",
"capture/return formatted utterance in validate_ssml
(templates/tts.py:validate_ssml -> format_speak_tags)", "log exceptions in
stop() (templates/tts.py:stop)", "avoid class-level mutable TTS.queue/playback
(templates/tts.py:TTS)", "ensure extract_speech always returns bytes or
annotated Optional (templates/vad.py:extract_speech)", "fix constraints override
logic (installation.py)","plugin_type enum handling
(plugin_entry.py:OpenVoiceOSPlugin.plugin_type)", and "narrow except: to except
Exception in sort_plugin_configs (utils/config.py:sort_plugin_configs)". For
each issue include: brief description, repro steps, affected symbol(s) (use
function/class names above), suggested fix, severity label, and link to this
AUDIT.md; assign to the relevant owner or triage team and add
milestone/priority.
In `@docs/index.md`:
- Around line 29-36: Add a language specifier "text" to the fenced code block
that contains the ASCII architecture diagram (the block with "ovos-core /
ovos-dinkum-listener / ovos-audio" and the plugin-manager bullets) so the linter
recognizes it as plaintext; replace the opening ``` with ```text to silence the
static analysis warning and keep the diagram unchanged.
In `@docs/writing-plugins.md`:
- Around line 202-206: The infinite loop in the run method (the `while True:
pass` placeholder) should be annotated with a brief comment clarifying intent
and advising readers not to copy it verbatim; update the `run` function to
replace `while True: pass` with the placeholder comment (e.g., indicating this
is a stub for the daemon main loop and should be replaced with proper
work/sleep/signal handling) so it's clear to readers that this is an intentional
stub and not production code.
In `@ovos_plugin_manager/templates/audio.py`:
- Around line 254-263: The fallback block in the try/except around importing
OCPFilesMetadataExtractor currently swallows import failures; update the except
in ovos_plugin_manager/templates/audio.py to log the exception before returning
the fallback metadata: catch the Exception in the existing try/except that
surrounds "from ovos_ocp_files_plugin.plugin import OCPFilesMetadataExtractor"
and call the module logger (or processLogger) to record the error and context
(e.g. include the uri and exception message) prior to constructing/returning the
fallback meta dict used by OCPFilesMetadataExtractor.extract_metadata, keeping
PlaybackType and TrackState usage unchanged.
In `@ovos_plugin_manager/templates/solvers.py`:
- Around line 274-277: The except Exception: block that catches failures from
calling _call_with_sanitized_kwargs(self.get_data, query, lang=lang,
units=units) should log the caught exception before returning {} to aid
debugging; update the handler to call a logger (e.g., logger.exception(...) or
logging.exception(...)) including context like query, lang and units plus a
short message identifying self.get_data/_call_with_sanitized_kwargs, then return
{} as before.
In `@ovos_plugin_manager/templates/transformers.py`:
- Line 33: Update the type hint for the transform method's parameter to reflect
that None is allowed: change the signature of def transform(self, context: dict
= None) -> dict: to use Optional[dict] for the context parameter (e.g., def
transform(self, context: Optional[dict] = None) -> dict:); since Optional is
already imported, just adjust the annotation on the transform method to avoid
using bare None with dict.
- Around line 210-220: The docstring for feed_speech_utterance uses tabs for
indenting the "Parameters:" and "Returns:" lines; update the docstring in the
feed_speech_utterance method to use spaces (matching feed_speech_chunk) so
documentation generators render consistently, keeping the same parameter names
(chunk: bytes) and return description and leaving the return behavior calling
on_speech_end(chunk) unchanged.
- Around line 68-69: The transform method signature currently types the optional
context as plain dict; update transform(self, utterances: List[str], context:
dict = None) -> Tuple[...] to use Optional[dict] for the context parameter
(i.e., context: Optional[dict] = None) and add the corresponding import from
typing (Optional) if missing; apply the same change to the other transformer
signature mentioned (MetadataTransformer.transform) so both use Optional[dict].
In `@ovos_plugin_manager/templates/tts.py`:
- Around line 692-699: The stop() method on TTS currently swallows all
exceptions (except Exception as e: pass); update the except block to log the
caught exception instead of ignoring it so failures calling TTS.playback.stop()
are recorded. Locate the stop method (TTS.stop) and replace the silent pass with
a logging call (for example use self.log.exception or module logger.exception)
that includes a clear message and the exception details, keeping the surrounding
try/except and the subsequent self.add_metric({"metric_type": "tts.stop"})
intact.
In `@ovos_plugin_manager/triples.py`:
- Line 53: The return annotation for get_triples_module_configs is
redundant—replace Union[Dict[str, list], dict] with a single, clearer type such
as dict (or define a descriptive TypeAlias if you need to distinguish
"structured" vs "unstructured" returns); update the function signature for
get_triples_module_configs(module_name: str) to use that single type and, if
keeping two shapes is important, add a short comment or a TypeAlias name to
document the two possible return forms.
In `@ovos_plugin_manager/tts.py`:
- Line 93: Update the get_tts_config signature to use explicit Optional types
for parameters: import typing.Optional (or from typing import Optional) and
change the annotations to Optional[dict] for config and Optional[str] for module
so the function signature is get_tts_config(config: Optional[dict] = None,
module: Optional[str] = None) -> dict; update any related type hints or
docstrings if present to match the new annotations.
In `@test/unittests/test_audio_backend_template.py`:
- Around line 162-215: Tests currently only exercise load_track, ocp_start,
ocp_stop, ocp_error, ocp_pause, ocp_resume and ocp_sync_playback for lack of
exceptions; update each test to assert the actual OCP/bus event emitted: mock or
spy the backend bus emitter (e.g., patch backend.bus.emit or attach a capture
list) before calling
load_track/ocp_start/ocp_stop/ocp_error/ocp_pause/ocp_resume/ocp_sync_playback,
then assert the emitter was called with the expected event type/payload (for
example load_track sets _now_playing and emits a "load" or "now_playing" event,
ocp_start -> "playing", ocp_stop -> "stopped", ocp_error -> "error", ocp_pause
-> "paused", ocp_resume -> "playing", ocp_sync_playback -> a timing/sync event)
and that _now_playing is cleared or set as appropriate; use the backend methods'
names (load_track, ocp_start, ocp_stop, ocp_error, ocp_pause, ocp_resume,
ocp_sync_playback and _now_playing) to locate the logic to test.
In `@test/unittests/test_coreference_legacy.py`:
- Line 19: Remove the unused imports MagicMock and patch from the top of
test_coreference_legacy.py (the import statement that currently reads "from
unittest.mock import MagicMock, patch") so only actually used test utilities are
imported; if other mocks are needed later, import them at that time.
In `@test/unittests/test_embeddings_templates.py`:
- Line 41: The __init__ signature currently uses a default None with type
Dict[str, Any]; change the annotation to Optional[Dict[str, Any]] for the config
parameter (e.g., def __init__(self, config: Optional[Dict[str, Any]] = None) ->
None:) and add/import Optional from typing at the top of the file so the type is
explicit per PEP 484; update any related type hints in the same test module if
present to use Optional as well.
In `@test/unittests/test_media_templates.py`:
- Around line 362-375: Replace the issubclass assertions with
instantiation-based smoke checks: in tests like
test_remote_audio_is_audio_backend, test_remote_video_is_video_backend, and
test_remote_web_is_web_backend attempt to construct RemoteAudioPlayerBackend,
RemoteVideoPlayerBackend, and RemoteWebPlayerBackend respectively (using any
required minimal constructor args or simple mocks) and assert the instances are
of the expected base types (AudioPlayerBackend, VideoPlayerBackend,
WebPlayerBackend); this will fail if abstract methods are unimplemented and thus
ensures concrete/instantiable implementations rather than only subclass
declarations.
In `@test/unittests/test_ocp_extended.py`:
- Around line 72-79: Update the test_load_plugins_failure_skipped test to
exercise the "continue loading" path by returning a mixed plugin set from the
patched find_ocp_plugins: include one plugin class that raises (bad_cls =
MagicMock(side_effect=RuntimeError("fail"))) and one real/simple successful
plugin class (e.g., a minimal callable or MagicMock that returns an instance).
Instantiate StreamHandler(), call its load() path (via constructor or explicit
load method) and assert that the failing plugin key is not in handler.extractors
while the succeeding plugin key is present, verifying StreamHandler.load()
continues after exceptions.
In `@test/unittests/test_phal_template_extended.py`:
- Around line 55-338: The tests in TestPHALPluginMouthEvents and
TestPHALPluginEventHandlers are repetitive; replace the many nearly-identical
test methods with table-driven loops using subTest to iterate handler names and
expected delegate behavior: for mouth events build a list of tuples mapping
mouth callbacks (e.g. "_on_mouth_talk", "_on_mouth_think", "_on_mouth_listen",
"_on_mouth_smile", "_on_mouth_viseme", "_on_mouth_viseme_list",
"_on_mouth_reset") to their delegate names ("on_talk", "on_think", "on_listen",
"on_smile", "on_viseme", "on_viseme_list", "on_display_reset"), then in a single
test iterate with self.subTest, toggle plugin._activate_mouth_events() /
_deactivate_mouth_events(), patch the delegate via setattr to MagicMock and call
getattr(plugin, mouth_cb)(MagicMock()) asserting called/not called; similarly
for the no-op handlers in TestPHALPluginEventHandlers create a list of handler
names (on_record_begin, on_record_end, on_audio_output_start, ..., run) and loop
with subTest calling getattr(self.plugin, handler)() to ensure no exception;
keep the helper methods _activate/_deactivate and reuse them to set state.
In `@test/unittests/test_pipeline.py`:
- Line 4: Remove the unused import by editing the import statement that
currently reads "from ovos_plugin_manager.utils import PluginTypes,
PluginConfigTypes" and drop "PluginConfigTypes" so it only imports
"PluginTypes"; this removes the unused symbol PluginConfigTypes and fixes the
linter warning.
In `@test/unittests/test_solver.py`:
- Around line 457-478: The test adds assert_has_calls and call_count checks for
test_auto_translate_decorator but the assertions still use the long
unittest.mock.call qualifier; import call from unittest.mock and replace
unittest.mock.call(...) with call(...) in the assertions inside
test_auto_translate_decorator (references: test_auto_translate_decorator and the
nested test_func) to simplify the lines and improve readability; do not change
the function signature of test_func even though solver and lang appear unused.
In `@test/unittests/test_sr_audiodata.py`:
- Around line 25-29: The helper _make_audio_data declares a sample_width
parameter that is misleading because the struct.pack format "<{num_samples}h"
always creates 2-byte samples; either remove the sample_width parameter from
_make_audio_data and always construct frame_data as 2-byte samples and call
srAudioData(..., sample_width=2), or if you need variable widths, implement
packing logic that switches format based on sample_width (e.g., 'b' for 1, 'h'
for 2, 'i' for 4) and validate/raise on unsupported widths; update the
_make_audio_data signature and all tests that call it (or add an assertion
inside _make_audio_data that sample_width == 2) so behavior and parameters
remain consistent with the produced frame_data and srAudioData usage.
In `@test/unittests/test_thirdparty_solvers.py`:
- Around line 213-245: Update the tests to assert the exact arguments forwarded
to the translator/detector mocks instead of only call counts: in
test_translate_calls_translator replace mock_tx.translate.assert_called_once()
with mock_tx.translate.assert_called_once_with("hello", target_lang="fr",
source_lang="en"); in test_same_lang_skips_translation keep
mock_tx.translate.assert_not_called(); in test_auto_detect_source_lang add
mock_det.detect.assert_called_once_with("bonjour") and replace
mock_tx.translate.assert_called_once() with
mock_tx.translate.assert_called_once_with("bonjour", target_lang="en",
source_lang="fr"); in test_translate_list replace
mock_tx.translate_list.assert_called_once() with
mock_tx.translate_list.assert_called_once_with(["x", "y"], target_lang="fr") and
in test_translate_dict replace mock_tx.translate_dict.assert_called_once() with
mock_tx.translate_dict.assert_called_once_with({"k": "v"}, target_lang="fr").
In `@test/unittests/test_thirdparty_sr.py`:
- Around line 43-47: The helper _make_audio_data uses a misleading parameter
name sample_width; rename it to bytes_per_sample (or sample_bytes) and update
the docstring to state "bytes per sample" (default 2) and keep passing that
value as the third argument to srAudioData so it correctly represents
bytes-per-sample; also update any tests calling _make_audio_data to use the new
parameter name for consistency with srAudioData and match the fix applied in
test_sr_audiodata.py.
- Around line 222-278: The tests in TestSrAudioDataGetSegment (methods testing
srAudioData.get_segment) and TestSrAudioDataRateConversion (methods testing
srAudioData.get_raw_data/convert_rate) duplicate coverage already present in
test_sr_audiodata.py; consolidate by removing or relocating these duplicate test
methods into the existing test_sr_audiodata.py suite. Specifically, either
delete the classes TestSrAudioDataGetSegment and TestSrAudioDataRateConversion
from this file, or move the unique assertions (e.g., test_segment_both,
test_convert_rate) into the existing tests that target srAudioData.get_segment
and srAudioData.get_raw_data in test_sr_audiodata.py, keeping references to
get_segment, get_raw_data, and helper _make_audio_data to preserve setup and
assertions.
In `@test/unittests/test_tokenization_template.py`:
- Around line 91-94: The test test_runtime_requirements currently only asserts
that _WordTokenizer.runtime_requirements is not None; update it to also assert
the object is an instance of the expected type (RuntimeRequirements) by
importing or referencing the RuntimeRequirements class and using an
isinstance-style assertion (e.g., assertIsInstance) against
_WordTokenizer.runtime_requirements so the test validates both presence and
type.
- Around line 76-89: The tests test_restore_spans and
test_restore_spans_with_gap use assertIn and should instead assert the exact
reconstructed text from Tokenizer.restore_spans; update test_restore_spans to
assertEqual(Tokenizer.restore_spans(spans), "hello world") using the spans from
self.tok.span_tokenize("hello world"), and update test_restore_spans_with_gap to
assertEqual(Tokenizer.restore_spans([(0,5,"hello"), (10,15,"world")]), "hello
world") to confirm the gap case inserts a single space between tokens.
- Around line 54-57: Update the test_lang_from_config unit test to assert the
concrete expected value from the config rather than only its type: after
retrieving lang via self.tok.lang (in test_lang_from_config), add an assertion
that lang == "en-us" (or the canonical config value) so the test verifies the
actual configured locale string rather than just instance type.
In `@test/unittests/test_tts_template_extended.py`:
- Around line 255-270: The test_synth method assigns cache =
ctx.get_cache(cache_config=cache_config) but never uses it; remove the unused
cache variable or assert something about it (e.g., verify type or properties) to
avoid lint warnings. Update test_synth to either delete the call to
ctx.get_cache(...) and the cache variable, or replace it with an assertion that
uses cache (referencing ctx.get_cache and the cache variable) to validate
expected behavior before calling self.tts.get_tts.
- Around line 286-311: The three tests (test_validate_does_not_raise,
test_validate_lang_does_not_raise, test_validate_connection_does_not_raise)
currently swallow all exceptions with try/except: pass; change them to either
assert the methods are callable (e.g., assertTrue(callable(validator.validate)))
when you only need existence, or catch specific expected exceptions and call
self.skipTest(...) for environment-dependent failures (e.g., catch
ToolNotFoundError or the specific exception thrown by
validate/validate_lang/validate_connection) so real failures surface; update
references to TTSValidator.validate, TTSValidator.validate_lang, and
TTSValidator.validate_connection accordingly.
In `@test/unittests/test_tts_template_streaming.py`:
- Around line 246-253: The test test_viseme_without_duration uses an unused loop
variable named "code"; rename it to "_code" in the for loop (for _code, dur in
result:) to signal it's intentionally unused and silence the linter warning
while leaving the rest of the test logic (checking dur via assertAlmostEqual)
unchanged.
- Line 17: Remove the unused import by deleting the "import asyncio" statement
in the test_tts_template_streaming module; locate the top-of-file import block
where "asyncio" is imported and simply remove that import so the file no longer
contains an unused reference to asyncio.
- Around line 372-381: Replace hardcoded "/tmp/out.wav" in
test_get_tts_calls_sentence_to_files with a tempfile-generated path: create a
temporary file (e.g., via tempfile.NamedTemporaryFile or tempfile.mkstemp) and
use its name when calling tts.get_tts and asserting returned wav; keep the
MagicMock setup on _DummyConcatTTS.sentence_to_files and .concat and ensure
cleanup/closing of the temp file if needed. Reference symbols:
test_get_tts_calls_sentence_to_files, _DummyConcatTTS, sentence_to_files,
concat, get_tts.
In `@test/unittests/test_utils.py`:
- Line 799: The side_effect assigned to mock_load_cfgs uses an unused parameter
named ptype; update the lambda to use a conventional unused parameter name
(e.g., replace "ptype" with "_" or "_ptype") in the mock_load_cfgs.side_effect
assignment so the intent is clear and linter warnings are avoided (locate the
mock_load_cfgs.side_effect lambda in test_utils.py and change the parameter name
accordingly).
- Around line 608-611: The test currently only asserts length after writing to
ReadWriteStream(max_size=4); add an assertion to verify which bytes remain to
confirm trimming behavior: after rws2.write(b"12345678") assert the buffer
content via rws2.read(-1) (or equivalent accessor) matches the expected kept
bytes (e.g., b"5678" if oldest bytes are dropped, or b"1234" if newest are
dropped) so the test validates both size and content.
- Line 761: The test's mock side effect uses an unused lambda parameter `ptype`
in the assignment to mock_load_cfgs; update the lambda signature to use a
conventional discard name (e.g., use `_` or `*_` for the second parameter) so
static analysis stops flagging the unused parameter, leaving the logic
(returning {"en-US": []} when name == "plug-a" else {}) unchanged for
mock_load_cfgs.
- Line 775: The lambda used as the side_effect for mock_load_cfgs declares an
unused parameter named ptype; update the lambda signature to use `_` (or
`_ptype`) instead of `ptype` to indicate it's intentionally unused and satisfy
static analysis — e.g. change the side_effect lambda currently referenced by
mock_load_cfgs to accept (name, _) rather than (name, ptype) and keep the
existing return dict unchanged.
Comment on lines
+35
to
+180
| class _ConcreteMediaBackend(MediaBackend): | ||
| """Minimal concrete MediaBackend for testing.""" | ||
|
|
||
| def supported_uris(self): | ||
| """Return supported URI types.""" | ||
| return ["file", "http"] | ||
|
|
||
| def play(self): | ||
| """No-op play.""" | ||
|
|
||
| def stop(self): | ||
| """No-op stop.""" | ||
|
|
||
| def pause(self): | ||
| """No-op pause.""" | ||
|
|
||
| def resume(self): | ||
| """No-op resume.""" | ||
|
|
||
| def lower_volume(self): | ||
| """No-op lower_volume.""" | ||
|
|
||
| def restore_volume(self): | ||
| """No-op restore_volume.""" | ||
|
|
||
| def get_track_length(self) -> int: | ||
| """Return zero track length.""" | ||
| return 0 | ||
|
|
||
| def get_track_position(self) -> int: | ||
| """Return zero position.""" | ||
| return 0 | ||
|
|
||
| def set_track_position(self, milliseconds) -> None: | ||
| """No-op set_track_position.""" | ||
|
|
||
|
|
||
| class _ConcreteAudioBackend(AudioPlayerBackend): | ||
| """Minimal AudioPlayerBackend for testing.""" | ||
|
|
||
| def supported_uris(self): | ||
| """Return audio URIs.""" | ||
| return ["file"] | ||
|
|
||
| def play(self): | ||
| """No-op play.""" | ||
|
|
||
| def stop(self): | ||
| """No-op stop.""" | ||
|
|
||
| def pause(self): | ||
| """No-op pause.""" | ||
|
|
||
| def resume(self): | ||
| """No-op resume.""" | ||
|
|
||
| def lower_volume(self): | ||
| """No-op lower_volume.""" | ||
|
|
||
| def restore_volume(self): | ||
| """No-op restore_volume.""" | ||
|
|
||
| def get_track_length(self) -> int: | ||
| """Return zero.""" | ||
| return 0 | ||
|
|
||
| def get_track_position(self) -> int: | ||
| """Return zero.""" | ||
| return 0 | ||
|
|
||
| def set_track_position(self, milliseconds) -> None: | ||
| """No-op.""" | ||
|
|
||
|
|
||
| class _ConcreteVideoBackend(VideoPlayerBackend): | ||
| """Minimal VideoPlayerBackend for testing.""" | ||
|
|
||
| def supported_uris(self): | ||
| """Return video URIs.""" | ||
| return ["file"] | ||
|
|
||
| def play(self): | ||
| """No-op play.""" | ||
|
|
||
| def stop(self): | ||
| """No-op stop.""" | ||
|
|
||
| def pause(self): | ||
| """No-op pause.""" | ||
|
|
||
| def resume(self): | ||
| """No-op resume.""" | ||
|
|
||
| def lower_volume(self): | ||
| """No-op lower_volume.""" | ||
|
|
||
| def restore_volume(self): | ||
| """No-op restore_volume.""" | ||
|
|
||
| def get_track_length(self) -> int: | ||
| """Return zero.""" | ||
| return 0 | ||
|
|
||
| def get_track_position(self) -> int: | ||
| """Return zero.""" | ||
| return 0 | ||
|
|
||
| def set_track_position(self, milliseconds) -> None: | ||
| """No-op.""" | ||
|
|
||
|
|
||
| class _ConcreteWebBackend(WebPlayerBackend): | ||
| """Minimal WebPlayerBackend for testing.""" | ||
|
|
||
| def supported_uris(self): | ||
| """Return web URIs.""" | ||
| return ["http", "https"] | ||
|
|
||
| def play(self): | ||
| """No-op play.""" | ||
|
|
||
| def stop(self): | ||
| """No-op stop.""" | ||
|
|
||
| def pause(self): | ||
| """No-op pause.""" | ||
|
|
||
| def resume(self): | ||
| """No-op resume.""" | ||
|
|
||
| def lower_volume(self): | ||
| """No-op lower_volume.""" | ||
|
|
||
| def restore_volume(self): | ||
| """No-op restore_volume.""" | ||
|
|
||
| def get_track_length(self) -> int: | ||
| """Return zero.""" | ||
| return 0 | ||
|
|
||
| def get_track_position(self) -> int: | ||
| """Return zero.""" | ||
| return 0 | ||
|
|
||
| def set_track_position(self, milliseconds) -> None: | ||
| """No-op.""" |
Contributor
There was a problem hiding this comment.
Record hook calls instead of using pure no-op helpers.
Because these concrete backends make every lifecycle/seek hook a no-op and always report position 0, a lot of the tests below are effectively just “does not raise” smoke checks. Regressions where ocp_start stops calling play(), shutdown() stops calling stop(), or seek_*() computes the wrong offset would still pass.
Example of tightening one helper and the affected assertions
class _ConcreteMediaBackend(MediaBackend):
"""Minimal concrete MediaBackend for testing."""
+
+ def __init__(self, *args, **kwargs):
+ super().__init__(*args, **kwargs)
+ self.play_calls = 0
+ self.stop_calls = 0
+ self.pause_calls = 0
+ self.resume_calls = 0
+ self.last_track_position = None
def play(self):
- """No-op play."""
+ self.play_calls += 1
def stop(self):
- """No-op stop."""
+ self.stop_calls += 1
def pause(self):
- """No-op pause."""
+ self.pause_calls += 1
def resume(self):
- """No-op resume."""
+ self.resume_calls += 1
def get_track_position(self) -> int:
- """Return zero position."""
- return 0
+ return self.last_track_position or 0
def set_track_position(self, milliseconds) -> None:
- """No-op set_track_position."""
+ self.last_track_position = milliseconds def test_ocp_start(self) -> None:
self.backend._now_playing = "file:///test.mp3"
- self.backend.ocp_start() # Should not raise
+ self.backend.ocp_start()
+ self.assertEqual(self.backend.play_calls, 1)
def test_seek_forward(self) -> None:
- self.backend.seek_forward(1) # get_track_position() + 1000ms
+ self.backend.seek_forward(1)
+ self.assertEqual(self.backend.last_track_position, 1000)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/unittests/test_media_templates.py` around lines 35 - 180, The test
backends (_ConcreteMediaBackend, _ConcreteAudioBackend, _ConcreteVideoBackend,
_ConcreteWebBackend) use pure no-op lifecycle/seek hooks and always return
position 0, which hides regressions; change these classes to record calls and
simulate realistic track position changes: implement
play/stop/pause/resume/lower_volume/restore_volume to append events to an
internal list (e.g., self.calls) and update a mutable position state, implement
set_track_position to update that state, and make
get_track_position/get_track_length return the simulated values so tests can
assert that ocp_start -> play(), shutdown -> stop(), and seek_* compute offsets
correctly by inspecting the recorded calls and positions.
Comment on lines
+211
to
+224
| def test_load_track(self) -> None: | ||
| """load_track sets _now_playing and emits media state message.""" | ||
| self.backend.load_track("file:///test.mp3") | ||
| self.assertEqual(self.backend._now_playing, "file:///test.mp3") | ||
|
|
||
| def test_load_track_with_metadata(self) -> None: | ||
| """load_track stores metadata.""" | ||
| self.backend.load_track("file:///test.mp3", metadata={"title": "Test"}) | ||
| self.assertEqual(self.backend.meta["title"], "Test") | ||
|
|
||
| def test_ocp_start(self) -> None: | ||
| """ocp_start calls play and emits player state.""" | ||
| self.backend._now_playing = "file:///test.mp3" | ||
| self.backend.ocp_start() # Should not raise |
Contributor
There was a problem hiding this comment.
Assert the queued/playing state emission explicitly.
These tests say they verify QUEUED_* / PLAYING_* behavior, but they only assert _now_playing or that no exception was raised. If load_track() / ocp_start() / ocp_pause() stop publishing the state update, this suite still passes. Please patch the message/bus emitter used by these base classes and assert the emitted state for each backend type.
Also applies to: 248-261, 303-355
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/unittests/test_media_templates.py` around lines 211 - 224, The tests
currently only check side effects like backend._now_playing and backend.meta but
must also assert that the message/bus emitter receives the correct QUEUED_* and
PLAYING_* state events; update the tests for test_load_track,
test_load_track_with_metadata, test_ocp_start (and the other ranges noted) to
patch/mock the emitter used by the media backend (the same emitter the base
class uses) and add assertions that a state message with the expected QUEUED_*
topic is emitted after load_track and that a PLAYING_* message is emitted when
ocp_start is called (and analogous checks for ocp_pause); locate calls to
load_track, ocp_start, ocp_pause and the backend instance in these tests and
replace or wrap the emitter to capture published messages, then assert on the
captured messages' topic/payload along with the existing assertions on
_now_playing/meta.
Comment on lines
+103
to
+128
| def test_extract_stream_with_sei_extractor(self, mock_find: MagicMock) -> None: | ||
| """extract_stream uses SEI extractor when uri matches.""" | ||
| from ovos_plugin_manager.ocp import StreamHandler | ||
| mock_extractor = MagicMock() | ||
| mock_extractor.supported_seis = ["yt"] | ||
| mock_extractor.extract_stream.return_value = {"uri": "https://real-stream.com/v.mp4"} | ||
| mock_extractor.validate_uri.return_value = False | ||
| mock_cls = MagicMock(return_value=mock_extractor) | ||
| mock_find.return_value = {"yt-plug": mock_cls} | ||
| handler = StreamHandler() | ||
| result = handler.extract_stream("yt//https://youtube.com/watch?v=123") | ||
| self.assertIn("uri", result) | ||
|
|
||
| @patch("ovos_plugin_manager.ocp.find_ocp_plugins") | ||
| def test_extract_stream_url_extractor(self, mock_find: MagicMock) -> None: | ||
| """extract_stream falls back to URL-matching extractor.""" | ||
| from ovos_plugin_manager.ocp import StreamHandler | ||
| mock_extractor = MagicMock() | ||
| mock_extractor.supported_seis = [] | ||
| mock_extractor.validate_uri.return_value = True | ||
| mock_extractor.extract_stream.return_value = {"uri": "https://stream.com/audio.m3u8"} | ||
| mock_cls = MagicMock(return_value=mock_extractor) | ||
| mock_find.return_value = {"url-plug": mock_cls} | ||
| handler = StreamHandler() | ||
| result = handler.extract_stream("https://example.com/video") | ||
| self.assertIn("uri", result) |
Contributor
There was a problem hiding this comment.
Strengthen these assertions so they prove the extractor path ran.
Both tests pass on the raw fallback path because extract_stream() always returns a dict with a "uri" key. Assert the exact transformed URI and the relevant mock interactions, otherwise these tests do not actually protect the SEI/URL dispatch behavior they describe.
Suggested change
def test_extract_stream_with_sei_extractor(self, mock_find: MagicMock) -> None:
"""extract_stream uses SEI extractor when uri matches."""
from ovos_plugin_manager.ocp import StreamHandler
mock_extractor = MagicMock()
mock_extractor.supported_seis = ["yt"]
mock_extractor.extract_stream.return_value = {"uri": "https://real-stream.com/v.mp4"}
mock_extractor.validate_uri.return_value = False
mock_cls = MagicMock(return_value=mock_extractor)
mock_find.return_value = {"yt-plug": mock_cls}
handler = StreamHandler()
result = handler.extract_stream("yt//https://youtube.com/watch?v=123")
- self.assertIn("uri", result)
+ self.assertEqual(result, {"uri": "https://real-stream.com/v.mp4"})
+ mock_extractor.extract_stream.assert_called_once_with(
+ "yt//https://youtube.com/watch?v=123", True
+ )
`@patch`("ovos_plugin_manager.ocp.find_ocp_plugins")
def test_extract_stream_url_extractor(self, mock_find: MagicMock) -> None:
"""extract_stream falls back to URL-matching extractor."""
from ovos_plugin_manager.ocp import StreamHandler
mock_extractor = MagicMock()
mock_extractor.supported_seis = []
mock_extractor.validate_uri.return_value = True
mock_extractor.extract_stream.return_value = {"uri": "https://stream.com/audio.m3u8"}
mock_cls = MagicMock(return_value=mock_extractor)
mock_find.return_value = {"url-plug": mock_cls}
handler = StreamHandler()
result = handler.extract_stream("https://example.com/video")
- self.assertIn("uri", result)
+ self.assertEqual(result, {"uri": "https://stream.com/audio.m3u8"})
+ mock_extractor.validate_uri.assert_called_once_with("https://example.com/video")
+ mock_extractor.extract_stream.assert_called_once_with(
+ "https://example.com/video", True
+ )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/unittests/test_ocp_extended.py` around lines 103 - 128, The tests for
StreamHandler.extract_stream are too weak because they only assert presence of
"uri" (always present); update the assertions to verify the specific transformed
URIs returned by the mocked extractor (e.g. expect
"https://real-stream.com/v.mp4" for the SEI extractor case and
"https://stream.com/audio.m3u8" for the URL extractor case) and assert the
extractor interactions: confirm mock_extractor.extract_stream was called once
with the original URI and mock_cls (or other extractor mocks) were/not called as
appropriate to prove the SEI vs URL dispatch path was taken.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ovos_plugin_manager/installation.py (1)
103-107:⚠️ Potential issue | 🟠 MajorBug: User-provided constraints file is overwritten when
DEFAULT_CONSTRAINTSexists.When a valid
constraintspath is provided and exists, theelifbranch still executes ifDEFAULT_CONSTRAINTSalso exists, overwriting the user's choice. The condition should only apply the default when no constraints were explicitly provided.🐛 Suggested fix
if constraints and not exists(constraints): LOG.error('Couldn\'t find the constraints file') return False - elif exists(DEFAULT_CONSTRAINTS): + elif not constraints and exists(DEFAULT_CONSTRAINTS): constraints = DEFAULT_CONSTRAINTS🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovos_plugin_manager/installation.py` around lines 103 - 107, The code overwrites a user-provided constraints path because the branch "elif exists(DEFAULT_CONSTRAINTS)" runs even when `constraints` is set; change the logic so the default is applied only when no explicit `constraints` was provided (e.g. replace that branch with a condition like `if not constraints and exists(DEFAULT_CONSTRAINTS): constraints = DEFAULT_CONSTRAINTS`), keeping the existing check that returns False when `constraints` is provided but missing; adjust references to `constraints`, `DEFAULT_CONSTRAINTS`, and `exists` in installation.py accordingly.ovos_plugin_manager/plugin_entry.py (1)
70-99:⚠️ Potential issue | 🟠 MajorDon't return a live class object from
json.Line 97 puts
self.clazzinto a payload documented as JSON-serializable. For installed plugins,json.dumps(plugin.json)will fail because Python class objects are not serializable.Suggested change
+ clazz = self.clazz data = { "name": self.name, "package_name": self.package_name, "module_name": self.module_name, "human_name": self.human_name, "description": self.description, "plugin_type": self.plugin_type, "url": self.url, - "is_installed": self.is_installed, - "class": self.clazz + "is_installed": clazz is not None, + "class": self._data.get("class") or (clazz.__name__ if clazz else None) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovos_plugin_manager/plugin_entry.py` around lines 70 - 99, The json method currently places the live class object self.clazz into the returned dict which breaks JSON serialization; change the "class" value to a JSON-safe identifier (e.g. None if self.clazz is None or a fully-qualified name like f"{self.clazz.__module__}.{self.clazz.__name__}" or at minimum self.clazz.__name__) before calling merge_dict so the returned payload is JSON-serializable; update the assignment in json (referencing self.clazz and merge_dict) to use that string instead of the class object.
♻️ Duplicate comments (3)
ovos_plugin_manager/templates/vad.py (1)
104-111:⚠️ Potential issue | 🟡 MinorBroaden the
Nonereturn-path docs forextract_speech().Line 109-Line 110 currently describes
Noneonly for the “still triggered at end of buffer” case, but the implementation also returnsNonewhen speech is never detected at all, which is covered intest/unittests/test_vad.py:91-98. The docstring should reflect both cases so callers do not infer a stronger guarantee than the method provides.📝 Suggested doc fix
def extract_speech(self, audio: bytes) -> Optional[bytes]: """Returns the audio data with speech only, removing silence before and after. This method uses a sliding ring buffer to detect speech. It returns the - voiced segments once it detects enough trailing silence. If the input - buffer ends while speech is still being detected (the "triggered" state), - it returns None. + voiced segments once it detects enough trailing silence. It may return + ``None`` if speech is never detected, or if the input ends before the + trailing-unvoiced threshold is reached. """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovos_plugin_manager/templates/vad.py` around lines 104 - 111, The docstring for extract_speech currently states it returns None only when the VAD is still "triggered" at the end of the input buffer; update the extract_speech(...) docstring to also document the other None return-path: when no speech is ever detected in the input buffer (so callers know None can mean “no voiced segments found” as well as “still triggered at end”). Reference the extract_speech method name when making this change.ovos_plugin_manager/installation.py (1)
126-138:⚠️ Potential issue | 🟡 Minor
PipExceptionwill have empty stdout/stderr whenprint_logs=True.When
print_logs=True, lines 128-131 consume the stdout/stderr streams by iterating over them. The subsequentproc.stdout.read()at line 134 returns an empty string since the stream was already exhausted. The exception is raised with empty content despite logs being printed.This may be acceptable if the intent is "either print to terminal OR capture for exception", but the docstring implies the exception should contain the error details regardless.
🛠️ Suggested fix to capture output while printing
- proc = Popen(pip_command, stdout=PIPE, stderr=PIPE) + stdout_lines = [] + stderr_lines = [] + proc = Popen(pip_command, stdout=PIPE, stderr=PIPE) if print_logs: for line in proc.stdout: - print(line.decode().strip()) + decoded = line.decode().strip() + stdout_lines.append(decoded) + print(decoded) for line in proc.stderr: - print(line.decode().strip(), file=sys.stderr) + decoded = line.decode().strip() + stderr_lines.append(decoded) + print(decoded, file=sys.stderr) pip_code = proc.wait() if pip_code != 0: - stdout = proc.stdout.read().decode() if proc.stdout else "" - stderr = proc.stderr.read().decode() if proc.stderr else "" + if print_logs: + stdout = "\n".join(stdout_lines) + stderr = "\n".join(stderr_lines) + else: + stdout = proc.stdout.read().decode() if proc.stdout else "" + stderr = proc.stderr.read().decode() if proc.stderr else "" raise PipException( pip_code, stdout, stderr )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovos_plugin_manager/installation.py` around lines 126 - 138, When print_logs=True the code iterates over proc.stdout/proc.stderr which consumes those streams so later proc.stdout.read()/proc.stderr.read() returns empty and PipException lacks details; modify the install routine that spawns Popen(pip_command) to tee the output into buffers while printing (e.g., accumulate decoded lines from proc.stdout and proc.stderr into stdout_buf/stderr_buf as you print them) and after proc.wait() use those buffers when constructing PipException(pip_code, stdout_buf, stderr_buf) so the exception always contains the captured output even when print_logs is true; locate the logic around Popen, pip_command, print_logs, proc.stdout/proc.stderr and PipException to implement this change.ovos_plugin_manager/templates/tts.py (1)
264-268:⚠️ Potential issue | 🟡 MinorLog the tokenizer failure with traceback, not just a generic warning.
The fallback is fine, but this still hides the actual tokenizer error. Using
LOG.exception(...)here keeps the graceful fallback while preserving enough context to diagnose regressions later.Suggested fix
if self.config.get("sentence_tokenize"): # TODO default to True on next major release try: return quebra_frases.sentence_tokenize(sentence) except Exception: - LOG.warning("quebra_frases.sentence_tokenize failed, falling back to newline split") + LOG.exception("quebra_frases.sentence_tokenize failed; falling back to newline split") return sentence.split("\n")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovos_plugin_manager/templates/tts.py` around lines 264 - 268, The current except block in the TTS tokenization call swallows the original exception; change the fallback handler around quebra_frases.sentence_tokenize so it logs the full traceback using LOG.exception("quebra_frases.sentence_tokenize failed, falling back to newline split") instead of LOG.warning, then continue to return sentence.split("\n") as the graceful fallback; update the except block that surrounds quebra_frases.sentence_tokenize to call LOG.exception and preserve the existing return behavior.
🧹 Nitpick comments (2)
ovos_plugin_manager/installation.py (1)
64-65: Catch specific exception instead of bareException.The
int(p)conversion can only raiseValueError(orTypeErrorfor non-string). Catching the specific exception makes the intent clearer and avoids accidentally swallowing unrelated errors.♻️ Suggested fix
- except Exception: + except (ValueError, TypeError): raw_pages[idx] = 0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovos_plugin_manager/installation.py` around lines 64 - 65, The except block currently catches all exceptions around the int(p) conversion (raw_pages[idx] = 0); replace the bare except Exception with specific exceptions raised by int(p) — catch ValueError and TypeError instead — so change "except Exception:" to "except (ValueError, TypeError):" around the int(p) conversion that assigns raw_pages[idx].ovos_plugin_manager/plugin_entry.py (1)
53-67: Scope the metadata load to the resolved plugin type.Line 62 does an all-registry lookup even though Lines 54-61 already identified the registry. Passing the inferred type here avoids the extra scan and keeps this lookup unambiguous if two plugin groups ever reuse the same entry-point name.
Suggested change
- engine = load_plugin(name) + engine = load_plugin(name, plug_type=data.get("plugin_type"))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovos_plugin_manager/plugin_entry.py` around lines 53 - 67, The metadata loader currently calls load_plugin(name) which does a full registry scan even after find_* identified the type; instead pass the inferred PluginTypes to load_plugin to scope the lookup. After determining plugin_type (from find_stt_plugins/find_tts_plugins/find_wake_word_plugins/find_audio_service_plugins and storing it in data["plugin_type"]), call load_plugin with that type (e.g., load_plugin(name, data["plugin_type"]) or the appropriate second-argument expected by load_plugin) so the registry lookup is limited to the resolved plugin group before extracting engine.__name__ and engine.__doc__ for OpenVoiceOSPlugin.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/configuration.md`:
- Around line 123-124: The doc has a contradiction about priority direction:
reconcile it by choosing one convention and updating both places that mention
the "priority" key and the factory iteration behavior so they match;
specifically, decide whether lower numeric "priority" means higher precedence or
larger means higher precedence, then update the sentence that currently reads
'Sort each plugin's config list by the "priority" key (lower = higher priority)'
and the paragraph describing factory methods iterating 'from the end' so they
consistently state the same rule and explain how the factory iteration order
produces the final winner when duplicates occur; also add a short example
sentence showing the sort order (ascending or descending) to clarify for plugin
authors and ensure references to "priority" and "factory methods" in both places
are consistent.
In `@ovos_plugin_manager/plugin_entry.py`:
- Around line 21-38: The lazy-load currently only memoizes successful plugin
class resolution so repeated misses re-run discovery and re-log warnings; modify
the OpenVoiceOSPlugin wrapper (use the existing _clazz field and related
properties clazz, is_installed, and json and the load_plugin call) to cache
failed resolution by storing a sentinel (e.g., None/False or a dedicated
sentinel object) when load_plugin cannot find the class, and make the clazz
property treat that sentinel as “attempted and not found” so subsequent accesses
return the cached negative result without re-invoking discovery or re-emitting
warnings; ensure is_installed/json also check the sentinel and behave
consistently.
In `@ovos_plugin_manager/templates/tts.py`:
- Around line 365-368: The current guard only calls format_speak_tags when an
opening "<speak>" is present, so inputs with a stray closing tag like
"hello</speak>" bypass normalization; change the condition around the call that
uses self.format_speak_tags(utterance) (the block handling variable utterance)
to detect either an opening "<speak>" or a closing "</speak>" tag and invoke
format_speak_tags when either is found so dangling closing tags are normalized
as well.
In `@pyproject.toml`:
- Around line 2-3: The declared SPDX license string in pyproject.toml ("license
= \"Apache-2.0\"") is not supported by the setuptools floor currently specified
(setuptools>=61.0); update pyproject.toml to either raise the setuptools
requirement to "setuptools>=77.0.3" or change the license declaration to the
backward-compatible table form (license = { text = "Apache-2.0" }) so the
license syntax matches the available setuptools version; locate the existing
requires/build-backend lines and the license line in pyproject.toml and apply
one of these two fixes.
---
Outside diff comments:
In `@ovos_plugin_manager/installation.py`:
- Around line 103-107: The code overwrites a user-provided constraints path
because the branch "elif exists(DEFAULT_CONSTRAINTS)" runs even when
`constraints` is set; change the logic so the default is applied only when no
explicit `constraints` was provided (e.g. replace that branch with a condition
like `if not constraints and exists(DEFAULT_CONSTRAINTS): constraints =
DEFAULT_CONSTRAINTS`), keeping the existing check that returns False when
`constraints` is provided but missing; adjust references to `constraints`,
`DEFAULT_CONSTRAINTS`, and `exists` in installation.py accordingly.
In `@ovos_plugin_manager/plugin_entry.py`:
- Around line 70-99: The json method currently places the live class object
self.clazz into the returned dict which breaks JSON serialization; change the
"class" value to a JSON-safe identifier (e.g. None if self.clazz is None or a
fully-qualified name like f"{self.clazz.__module__}.{self.clazz.__name__}" or at
minimum self.clazz.__name__) before calling merge_dict so the returned payload
is JSON-serializable; update the assignment in json (referencing self.clazz and
merge_dict) to use that string instead of the class object.
---
Duplicate comments:
In `@ovos_plugin_manager/installation.py`:
- Around line 126-138: When print_logs=True the code iterates over
proc.stdout/proc.stderr which consumes those streams so later
proc.stdout.read()/proc.stderr.read() returns empty and PipException lacks
details; modify the install routine that spawns Popen(pip_command) to tee the
output into buffers while printing (e.g., accumulate decoded lines from
proc.stdout and proc.stderr into stdout_buf/stderr_buf as you print them) and
after proc.wait() use those buffers when constructing PipException(pip_code,
stdout_buf, stderr_buf) so the exception always contains the captured output
even when print_logs is true; locate the logic around Popen, pip_command,
print_logs, proc.stdout/proc.stderr and PipException to implement this change.
In `@ovos_plugin_manager/templates/tts.py`:
- Around line 264-268: The current except block in the TTS tokenization call
swallows the original exception; change the fallback handler around
quebra_frases.sentence_tokenize so it logs the full traceback using
LOG.exception("quebra_frases.sentence_tokenize failed, falling back to newline
split") instead of LOG.warning, then continue to return sentence.split("\n") as
the graceful fallback; update the except block that surrounds
quebra_frases.sentence_tokenize to call LOG.exception and preserve the existing
return behavior.
In `@ovos_plugin_manager/templates/vad.py`:
- Around line 104-111: The docstring for extract_speech currently states it
returns None only when the VAD is still "triggered" at the end of the input
buffer; update the extract_speech(...) docstring to also document the other None
return-path: when no speech is ever detected in the input buffer (so callers
know None can mean “no voiced segments found” as well as “still triggered at
end”). Reference the extract_speech method name when making this change.
---
Nitpick comments:
In `@ovos_plugin_manager/installation.py`:
- Around line 64-65: The except block currently catches all exceptions around
the int(p) conversion (raw_pages[idx] = 0); replace the bare except Exception
with specific exceptions raised by int(p) — catch ValueError and TypeError
instead — so change "except Exception:" to "except (ValueError, TypeError):"
around the int(p) conversion that assigns raw_pages[idx].
In `@ovos_plugin_manager/plugin_entry.py`:
- Around line 53-67: The metadata loader currently calls load_plugin(name) which
does a full registry scan even after find_* identified the type; instead pass
the inferred PluginTypes to load_plugin to scope the lookup. After determining
plugin_type (from
find_stt_plugins/find_tts_plugins/find_wake_word_plugins/find_audio_service_plugins
and storing it in data["plugin_type"]), call load_plugin with that type (e.g.,
load_plugin(name, data["plugin_type"]) or the appropriate second-argument
expected by load_plugin) so the registry lookup is limited to the resolved
plugin group before extracting engine.__name__ and engine.__doc__ for
OpenVoiceOSPlugin.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 076010b6-c945-4134-92e4-ded6787bf9d9
📒 Files selected for processing (9)
.github/workflows/build-tests.yml.github/workflows/release_workflow.ymlMAINTENANCE_REPORT.mddocs/configuration.mdovos_plugin_manager/installation.pyovos_plugin_manager/plugin_entry.pyovos_plugin_manager/templates/tts.pyovos_plugin_manager/templates/vad.pypyproject.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/build-tests.yml
Comment on lines
+21
to
38
| def __init__(self, data: dict): | ||
| """ | ||
| Create an OpenVoiceOSPlugin metadata wrapper from a raw metadata dictionary. | ||
|
|
||
| Parameters: | ||
| data (dict): Metadata dictionary for the plugin. Recognized keys: | ||
| - name: entry point name | ||
| - package_name: PyPI/pip package name | ||
| - module_name: Python module where the plugin class is defined | ||
| - human_name: display-friendly name | ||
| - description: textual description or docstring | ||
| - plugin_type: one of the plugin type identifiers (e.g., "tts", "stt", "wakeword", "audio") | ||
| - url: source or homepage URL | ||
| - class: plugin class name | ||
| """ | ||
| self._data = data | ||
| self._clazz = None | ||
| self._plugtype = None |
Contributor
There was a problem hiding this comment.
Cache failed lazy-load attempts too.
Line 280 only memoizes successful loads. When a plugin is missing, every later access to clazz, is_installed, or json re-runs discovery, and each miss can re-emit the same warning from load_plugin.
Suggested change
def __init__(self, data: dict):
"""
Create an OpenVoiceOSPlugin metadata wrapper from a raw metadata dictionary.
@@
self._data = data
self._clazz = None
self._plugtype = None
+ self._load_attempted = False
@@
`@property`
def clazz(self) -> Optional[Type]:
@@
- if not self._clazz and self.name:
+ if not self._load_attempted and self.name:
+ self._load_attempted = True
self._clazz = self.load()
return self._clazz
@@
def install(self) -> bool:
@@
if self.package_name:
- return pip_install([self.package_name])
+ ok = pip_install([self.package_name])
+ if ok:
+ self._load_attempted = False
+ self._clazz = None
+ return ok
if self.url and "github" in self.url:
- return pip_install(["git+" + self.url])
+ ok = pip_install(["git+" + self.url])
+ if ok:
+ self._load_attempted = False
+ self._clazz = None
+ return ok
return FalseAlso applies to: 273-309
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ovos_plugin_manager/plugin_entry.py` around lines 21 - 38, The lazy-load
currently only memoizes successful plugin class resolution so repeated misses
re-run discovery and re-log warnings; modify the OpenVoiceOSPlugin wrapper (use
the existing _clazz field and related properties clazz, is_installed, and json
and the load_plugin call) to cache failed resolution by storing a sentinel
(e.g., None/False or a dedicated sentinel object) when load_plugin cannot find
the class, and make the clazz property treat that sentinel as “attempted and not
found” so subsequent accesses return the cached negative result without
re-invoking discovery or re-emitting warnings; ensure is_installed/json also
check the sentinel and behave consistently.
Comment on lines
+2
to
+3
| requires = ["setuptools>=61.0", "wheel"] | ||
| build-backend = "setuptools.build_meta" |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Does setuptools 61.0 support license = "Apache-2.0"in[project]ofpyproject.toml? If not, what minimum setuptools version adds SPDX license string support, and what is the backward-compatible syntax for older setuptools versions?
💡 Result:
No. setuptools 61.0.0 (the first release with PEP 621 [project] support) does not support license = "Apache-2.0" as a bare string in [project]; SPDX license string support was added later with PEP 639 support. [1]
Minimum setuptools version for SPDX license strings
- Setuptools documentation: SPDX license expressions in
project.licensewere introduced in setuptools 77.0.0. [1] - Packaging guide’s backend-version table lists setuptools 77.0.3 as the version that introduced PEP 639 support. [2]
If you need a single “minimum” that matches the packaging guide, use setuptools>=77.0.3. [2]
Backward-compatible syntax for older setuptools (pre-77)
Use the (now-deprecated) PEP 621 table form:
[project]
license = { text = "Apache-2.0" }
# or, if you prefer embedding via a file:
license = { file = "LICENSE" }PEP 639 keeps the legacy license table keys (text/file) for backward compatibility, while the string form is the SPDX license expression form. [3]
Sources: setuptools pyproject.toml configuration docs [1], Python Packaging User Guide [2], PEP 639 [3].
Make the license field compatible with the declared setuptools floor.
Line 10 uses the SPDX string syntax license = "Apache-2.0", which is only supported by setuptools 77.0.3+. However, lines 2-3 declare setuptools>=61.0, causing potential build failures with older resolved versions.
Either bump the setuptools requirement to >=77.0.3 or use the backward-compatible table form license = { text = "Apache-2.0" }.
Backward-compatible fix for setuptools ≥61.0
-license = "Apache-2.0"
+license = { text = "Apache-2.0" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pyproject.toml` around lines 2 - 3, The declared SPDX license string in
pyproject.toml ("license = \"Apache-2.0\"") is not supported by the setuptools
floor currently specified (setuptools>=61.0); update pyproject.toml to either
raise the setuptools requirement to "setuptools>=77.0.3" or change the license
declaration to the backward-compatible table form (license = { text =
"Apache-2.0" }) so the license syntax matches the available setuptools version;
locate the existing requires/build-backend lines and the license line in
pyproject.toml and apply one of these two fixes.
When print_logs=True, output should go directly to console without piping. When print_logs=False, output should be piped to suppress it silently. Previously, the code always piped stdout/stderr and manually read and printed them when print_logs=True, which is inefficient and causes test failures. This fix: - Conditionally pipes output based on the print_logs flag - When print_logs=True, Popen is called without stdout/stderr pipes - When print_logs=False, Popen is called with PIPE to capture output silently Fixes: test_print_logs_true_no_pipe failure across all Python versions Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Add comprehensive unit tests: - test_intent_transformers.py: 100% coverage for intent_transformers.py - test_triples.py: 100% coverage for triples.py and templates/triples.py - Enhanced test_keywords.py: Added factory method tests for OVOSKeywordExtractorFactory - Enhanced test_tts.py: Added test for dangling closing tag normalization Bug fix in templates/tts.py: - Fixed validate_ssml to detect both opening "<speak>" and closing "</speak>" tags - Previously, dangling closing tags like "hello</speak>" were not normalized - Now properly calls format_speak_tags for both opening and closing tags Test results: - 990 tests passing - 85% code coverage - All new tests passing Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
…modules Add unit tests for plugin discovery functions: - test_postag.py: Complete test coverage for postag.py - test_segmentation.py: Complete test coverage for segmentation.py - test_tokenization.py: Complete test coverage for tokenization.py Tests follow the same pattern as other plugin discovery modules: - find_plugins tests - load_plugin tests - get_configs tests - get_module_configs tests - get_lang_configs tests - get_supported_langs tests Total tests: 21 new tests covering utility functions and template base classes Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Mark the following modules as deprecated with proper warnings and logging: 1. ovos_plugin_manager.installation - deprecated, will be removed in v3.0 - Added module-level deprecation log - Added @deprecated decorator to main functions: - search_pip() - pip_install() - Recommended: use modern package managers instead 2. ovos_plugin_manager.plugin_entry - deprecated, will be removed in v3.0 - Added module-level deprecation log - OpenVoiceOSPlugin class should no longer be used 3. ovos_plugin_manager.hardware - deprecated, will be removed in v3.0 - Module is being superseded by ovos-hardware-helpers package - Added deprecation log at module level - Affects: - ovos_plugin_manager.hardware.switches - ovos_plugin_manager.hardware.fan - ovos_plugin_manager.hardware.led Deprecation warnings include: - Deprecation version marker (3.0) - Clear guidance on what to use instead - Warning messages logged when modules are imported or functions called All existing tests pass. Deprecation warnings are properly logged via ovos_utils.log. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Update deprecation messages to dynamically calculate the next major version instead of hardcoding '3.0'. This ensures deprecation version stays accurate when the package version is updated. Changes: - installation.py: Use VERSION_MAJOR to determine deprecation version (3.0) - plugin_entry.py: Use VERSION_MAJOR to determine deprecation version (3.0) - hardware/__init__.py: Use VERSION_MAJOR to determine deprecation version (3.0) All deprecation notices now display as "removed in v3.0" dynamically calculated from VERSION_MAJOR (currently 2). When version is bumped to 3.x, deprecation version will automatically update to 4.0. Tests: All 990 tests passing Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Convert ovos_plugin_manager.hardware modules to act as backwards-compatibility shims that re-export classes from ovos-hardware-helpers. This directs users to the new canonical package while maintaining API compatibility. Changes: - fan.py: import AbstractFan from ovos_hardware_helpers.fan with deprecation log - switches.py: import AbstractSwitches from ovos_hardware_helpers.switches with deprecation log - led/__init__.py: import AbstractLed from ovos_hardware_helpers.led, maintain Color enum for backwards compatibility - hardware/__init__.py: add re-exports of all hardware classes for backwards-compatible imports All modules emit deprecation warnings on import, guiding users to migrate to ovos_hardware_helpers package. Tests verify backwards-compatibility is maintained. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
… imports Use module-level __getattr__ to implement lazy importing of ovos-hardware-helpers classes. This allows the shim modules to be imported without requiring ovos-hardware-helpers to be installed, while still providing helpful error messages if users try to access the hardware classes without the dependency. Deprecation warnings are still logged on module import to guide users toward the canonical ovos-hardware-helpers package. Only when a class is actually used does the import attempt happen. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Include ovos-hardware-helpers in the test dependencies so that hardware module tests can properly import the shim classes from ovos_plugin_manager.hardware. This ensures CI tests pass when running pytest, as the lazy imports need the dependency available when accessing the hardware classes. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Ensure test dependencies (including ovos-hardware-helpers) are installed when running coverage tests by passing install_extras: '.[test]' to the gh-automations coverage workflow. This fixes CI failures where hardware tests couldn't import the required ovos_hardware_helpers module. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
Documentation
Bug Fixes & Improvements
Tests