chore: Add comprehensive test suites and documentation#362
Merged
Conversation
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (3)
📒 Files selected for processing (13)
📝 WalkthroughWalkthroughThis PR comprehensively refactors CI/CD workflows to use shared automation, migrates packaging from setup.py to pyproject.toml, introduces a new service installer module, fixes error handling across multiple modules, adds extensive test coverage, and provides comprehensive documentation of the codebase. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes This review involves heterogeneous changes spanning CI/CD restructuring, package configuration migration, API additions/removals, bug fixes across multiple modules, type annotation additions, and extensive test coverage expansion. The skill_installer.py addition requires careful review for correctness of pip command construction and message-bus integration, while the signal.py removal and workflow migration demand verification that no breaking dependencies exist elsewhere. The scope of test additions is large but largely homogeneous and lower-risk. Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
|
- Replace inline build/publish/coverage jobs with reusable gh-automations workflows - Add missing workflows: build-tests, lint, license_check, pip_audit, release-preview, repo-health, python-support - Migrate from TigreGotico/gh-automations@master to OpenVoiceOS/gh-automations@dev - Replace codecov-based coverage.yml with coverage.yml@dev reusable workflow - Migrate downstream.yml from inline pipdeptree script to downstream-check.yml@dev - Remove deprecated build_tests.yml and unit_tests.yml (replaced by build-tests.yml) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
New test files:
- test_init.py: json_dumps, json_loads, datestr2ts (deprecated)
- test_text_utils.py: camel_case_split, collapse_whitespaces, rm_parentheses, remove_accents_and_punct
- test_list_utils.py: rotate_list, flatten_list, deduplicate_list
- test_decorators.py: classproperty, timed_lru_cache (with/without args, expiry, cache_clear)
- test_ocp.py: all enums, MediaEntry (CRUD, update, as_dict, from_dict, infocard, mimetype), PluginStream, Playlist (add/remove/sort/replace/navigate)
- test_fakebus.py: FakeMessage (serialize/deserialize, forward/reply/response/publish), FakeBus (emit, on/once/remove, wait_for_message/response, run_forever)
- test_ssml_extra.py: full SSMLBuilder coverage (sub, emphasis, parts_of_speech, pause, audio, prosody, pitch, volume, rate, phoneme, voice, whisper, remove_ssml, extract_ssml_tags)
Module coverage improvements:
- ssml.py: 67% → 94%
- decorators.py: 0% → 74%
- list_utils.py: 40% → 73%
- ocp.py: 15% → 26%
- fakebus.py: 26% → 41%
Also fix pyproject.toml: change license = "Apache" to license = {text = "Apache-2.0"}
to comply with PEP 621 / setuptools validation.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- oauth.py: inverted >= expiry check caused token refresh on every call;
missing None guard before expires_at; expires_in read from stale
pre-refresh dict instead of new_token_data
- network_utils.py: operator precedence bug — cfg.get("key" or fallback)
always passed first key; bare except swallowing KeyboardInterrupt
- skill_installer.py: proc.stderr.read() unguarded when stderr is None
- lang/__init__.py: split("-", 2) unpacked into 2 vars crashed on
3-part BCP-47 tags (e.g. zh-Hant-TW); bare excepts fixed
- log_parser.py: rstrip result discarded (newlines never stripped);
@classmethod used self instead of cls; open('w') truncated log file
when testing writability (changed to 'a'); bare excepts fixed
- geolocation.py: if lat and lon treated 0.0 as falsy, skipping valid
equator/meridian coordinates
- file_utils.py: _changed_files list → set for O(1) membership; bare except
- fakebus.py: bare except: → except Exception: (3 occurrences)
- device_input.py: bare except: → except Exception:
- xml_helper.py: bare except: → except Exception:
- security.py: unclosed file handles replaced with with-statements
- signal.py: type annotations added to all public deprecated functions
- skills.py: get_non_properties return type Set[str] added
- smtp_utils.py: full parameter and return type annotations added
- ocp.py: typing improvements throughout
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Expanded existing test stubs and added new test files across all major modules. Key additions: - test_oauth.py: full token lifecycle, expiry, refresh, and error paths - test_geolocation.py: mocked requests for all geocoding functions - test_log_parser.py: parsing, filtering, classmethod, writability tests - test_sound.py: sys.modules stubs for pygame/distutils (Python 3.13+) - test_device_input.py: evdev/xinput mocked; 100% coverage achieved - test_signal.py: deprecated module exercised via importlib reload - test_smtp_utils.py: SMTP_SSL mocked end-to-end - test_system.py: subprocess mocked for all system utility paths - test_gui.py: FakeBus-backed GUI helper tests - test_security_extra.py, test_process_utils_extra.py, test_network_utils_extra.py, test_file_utils_new.py: gap-filling - test_thread_utils.py, test_version.py, test_skills.py: new files 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, performance issues, type annotation gaps, and code smells from full source audit - SUGGESTIONS.md: architecture-level refactor proposals (log_parser CLI split, security module modernisation, etc.) - FAQ.md: keyword-rich Q&A covering common usage questions - QUICK_FACTS.md: machine-readable package reference (version, key classes, entry points) - MAINTENANCE_REPORT.md: AI-transparency log of all changes made - docs/index.md: repo overview and navigation Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
JarbasAl
force-pushed
the
docs_tests_automations
branch
from
bfeb038 to
8b34eb4
Compare
Contributor
I've completed my sweep! Here's the situation. 🧹I've aggregated the results of the automated checks for this PR below. 📋 Repo HealthEnsuring the codebase stays lean and mean. 💪 ✅ All required files present. Latest Version: ✅ 🔨 Build TestsThe build pipeline has finished its work. 🏁 ✅ All versions pass
🔍 LintThe automated results are now available for your perusal. 📂 ❌ ruff: issues found — see job log 📊 CoverageI've been crunching the numbers! Here's how the test coverage changed. 📈 Files below 80% coverage (26 files)
Full report: download the 🔒 Security (pip-audit)The security scan is now complete. 🏁 ✅ No known vulnerabilities found (45 packages scanned). ⚖️ License CheckScanning for any 'no-derivatives' clauses. 🚫 ✅ No license violations found (27 packages). License distribution: 8× MIT License, 5× MIT, 3× Apache Software License, 2× BSD-3-Clause, 2× ISC License (ISCL), 1× Apache Software License; BSD License, 1× Apache-2.0, 1× Apache-2.0 OR BSD-2-Clause, +4 more Full breakdown — 27 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. 🏷️ Release PreviewI've checked the 'Performance Improvements' metrics. 📈 Current:
✅ PR title follows conventional commit format. 🚀 Release Channel Compatibility Predicted next version:
Automating the boring stuff so you don't have to! 🚀 |
![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: 2
Note
Due to the large number of review comments, Critical 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 (1)
ovos_utils/log_parser.py (1)
158-186:⚠️ Potential issue | 🟠 MajorFlush a pending traceback at EOF.
If the file ends right after the exception line,
traceis never yielded. That drops the final traceback fromparse_file, soovos-logs slice/list -xcan silently miss the most recent failure.Proposed fix
def parse_file(cls, source) -> Generator[Union[LogLine, Traceback], None, None]: if not os.path.exists(source): raise FileNotFoundError(f"File {source} does not exist") with open(source, 'r') as file: trace = None last_timestamp = None for line in file: # gather all lines of the traceback if line == "Traceback (most recent call last):\n": trace = [line] continue # TODO do tracebacks always end on a empty line? elif trace and line == "\n": trace.append(line) traceback = Traceback.from_list(trace) traceback.timestamp = last_timestamp yield traceback trace = None elif trace: trace.append(line) else: log = cls.parse(line, last_timestamp) if not log.message.strip(): continue timestamp = log.timestamp if timestamp: last_timestamp = timestamp yield log + if trace: + traceback = Traceback.from_list(trace) + traceback.timestamp = last_timestamp + yield traceback🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovos_utils/log_parser.py` around lines 158 - 186, The parse_file generator in ovos_utils.log_parser.parse_file can drop a final traceback if the file ends while trace is non-None; after the for loop, check if trace is set and if so create a Traceback via Traceback.from_list(trace), set its timestamp to last_timestamp and yield it (same handling as inside the loop) so the pending traceback is flushed at EOF.
🟠 Major comments (19)
test/unittests/test_text_utils.py-61-78 (1)
61-78:⚠️ Potential issue | 🟠 MajorThese parentheses tests can pass on malformed output.
The current checks only prove some characters disappeared. A result like
"hello )"or extra whitespace churn would still pass, and Line 68’s.strip()masks regressions on the no-op path. Assert preserved text as well, and compare the untouched input without normalization.Proposed fix
def test_basic(self) -> None: result = rm_parentheses("hello (world)") self.assertNotIn("(", result) + self.assertNotIn(")", result) self.assertNotIn("world", result) + self.assertEqual(" ".join(result.split()), "hello") @@ def test_no_parens(self) -> None: - self.assertEqual(rm_parentheses("hello").strip(), "hello") + self.assertEqual(rm_parentheses("hello"), "hello") @@ def test_multiple(self) -> None: result = rm_parentheses("a (b) c (d)") self.assertNotIn("b", result) self.assertNotIn("d", result) + self.assertNotIn("(", result) + self.assertNotIn(")", result) + self.assertEqual(" ".join(result.split()), "a c") @@ def test_empty_parens(self) -> None: result = rm_parentheses("hello () world") self.assertNotIn("(", result) + self.assertNotIn(")", result) + self.assertEqual(" ".join(result.split()), "hello world")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_text_utils.py` around lines 61 - 78, The tests in TestRmParentheses are too loose: they only assert certain chars are absent or use .strip(), allowing malformed outputs like "hello )" or changed spacing to pass. Update tests for rm_parentheses to assert exact expected outputs (e.g., in test_basic assert result == "hello " or "hello" depending on desired trimming), remove the .strip() in test_no_parens and assert the result equals the original input, and in test_multiple and test_empty_parens assert the full resulting string equals the expected cleaned string (verifying both removed parentheses and preserved surrounding text/spacing) so the function's precise behavior is validated.test/unittests/test_text_utils.py-30-41 (1)
30-41:⚠️ Potential issue | 🟠 MajorStrengthen these camel-case tests with exact outputs.
test_acronymandtest_multiple_wordsonly check substrings, so unchanged or badly formatted output can still pass. These should pin the full split result.Proposed fix
def test_acronym(self) -> None: - result = camel_case_split("parseHTMLString") - self.assertIn("HTML", result) + self.assertEqual(camel_case_split("parseHTMLString"), + "parse HTML String") @@ def test_multiple_words(self) -> None: - result = camel_case_split("getFirstName") - self.assertIn("get", result) - self.assertIn("First", result) - self.assertIn("Name", result) + self.assertEqual(camel_case_split("getFirstName"), + "get First Name")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_text_utils.py` around lines 30 - 41, The tests for camel_case_split are too loose: replace the substring checks in test_acronym and test_multiple_words with exact equality assertions against the expected split lists/strings returned by camel_case_split; e.g., for the "parseHTMLString" case assert the full result equals the exact expected sequence (including "HTML" as a single token) and for "getFirstName" assert the result equals the exact list/sequence ["get","First","Name"] (or the exact string form your function returns), updating test_acronym and test_multiple_words to use assertEqual(camel_case_split(...), expected) instead of assertIn.test/unittests/test_text_utils.py-80-99 (1)
80-99:⚠️ Potential issue | 🟠 MajorPin the normalized text instead of fragments.
These tests mostly assert that some characters remain or disappear, which still allows truncated or reordered output to pass. Exact results are more appropriate for normalization helpers.
Proposed fix
def test_removes_accents(self) -> None: - result = remove_accents_and_punct("héllo") - self.assertNotIn("é", result) - self.assertIn("h", result) + self.assertEqual(remove_accents_and_punct("héllo"), "hello") @@ def test_removes_punctuation(self) -> None: - result = remove_accents_and_punct("hello, world!") - self.assertNotIn(",", result) - self.assertNotIn("!", result) + self.assertEqual(remove_accents_and_punct("hello, world!"), + "hello world") @@ def test_preserves_braces(self) -> None: - result = remove_accents_and_punct("{slot}") - self.assertIn("{", result) - self.assertIn("}", result) + self.assertEqual(remove_accents_and_punct("{slot}"), "{slot}") @@ def test_plain_text_unchanged(self) -> None: - result = remove_accents_and_punct("hello world") - self.assertIn("hello", result) - self.assertIn("world", result) + self.assertEqual(remove_accents_and_punct("hello world"), + "hello world")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_text_utils.py` around lines 80 - 99, The tests for remove_accents_and_punct are too loose; update each test to assert the full normalized output string instead of checking fragments. For test_removes_accents assert remove_accents_and_punct("héllo") == "hello"; for test_removes_punctuation assert remove_accents_and_punct("hello, world!") == "hello world"; for test_preserves_braces assert remove_accents_and_punct("{slot}") == "{slot}"; and for test_plain_text_unchanged assert remove_accents_and_punct("hello world") == "hello world" so the function's exact normalization behavior is verified.test/unittests/test_system.py-241-250 (1)
241-250:⚠️ Potential issue | 🟠 MajorAvoid depending on
python3existing on the runner.These assertions are environment-specific and will fail on platforms where the interpreter is exposed as
python/python.exerather thanpython3—notably Windows. Prefer deriving the executable name fromsys.executable, or mock the lookup so this stays a unit test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_system.py` around lines 241 - 250, The tests test_find_python and test_is_installed_python rely on a hardcoded "python3" executable which is environment-dependent; update them to derive the interpreter name from sys.executable (or its basename) or mock ovos_utils.system.find_executable/is_installed to avoid platform-specific failures. Specifically, change the test to use something like sys.executable (or os.path.basename(sys.executable)) as the argument passed to find_executable and is_installed, or patch/magicmock the find_executable/is_installed calls in those tests so they assert behavior without requiring an actual "python3" binary.test/unittests/test_system.py-147-154 (1)
147-154:⚠️ Potential issue | 🟠 MajorDon’t bake the known
check_service_installedbug into the test.This test explicitly switches to
sudo=Trueto sidestep the broken default branch, so CI can still pass while the commonsudo=False, user=Falsepath stays regressed. Please add a regression test for the default call and fix the implementation instead of codifying the workaround.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_system.py` around lines 147 - 154, The test currently avoids hitting the buggy default branch by calling check_service_installed("mysvc", sudo=True); instead add a regression test that calls check_service_installed("mysvc") (the default sudo=False,user=False path) and assert the same behavior (service suffix appended and proper return), and fix the implementation of check_service_installed so the local variable status_command is always initialized for the sudo=False,user=False branch (ensure the code path that builds the systemctl status command is executed or a fallback is assigned) and that the function still appends ".service" when the name lacks the suffix; update tests (including test_check_service_installed_appends_service) to validate both default and sudo=True behaviors.test/unittests/test_thread_utils.py-30-33 (1)
30-33:⚠️ Potential issue | 🟠 Major
autostart=Falseis not actually being validated.Both tests use a no-op target and only check
is_alive(). If the implementation incorrectly starts the thread immediately and it exits before the assertion, these tests still pass. Assert that the target has not run before an explicitstart().Suggested test shape
-from threading import Thread +from threading import Event, Thread @@ def test_autostart_false(self): from ovos_utils.thread_utils import create_daemon - t = create_daemon(target=lambda: None, autostart=False) + started = Event() + t = create_daemon(target=started.set, autostart=False) self.assertFalse(t.is_alive()) + self.assertFalse(started.is_set()) + t.start() + t.join(timeout=2.0) + self.assertTrue(started.is_set()) @@ def test_autostart_false(self): from ovos_utils.thread_utils import create_killable_daemon - t = create_killable_daemon(target=lambda: None, autostart=False) + started = Event() + t = create_killable_daemon(target=started.set, autostart=False) self.assertFalse(t.is_alive()) + self.assertFalse(started.is_set()) + t.start() + t.join(timeout=2.0) + self.assertTrue(started.is_set())Also applies to: 67-70
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_thread_utils.py` around lines 30 - 33, The tests for create_daemon (specifically test_autostart_false and the similar test at lines 67-70) only check t.is_alive() and can miss a thread that started and finished before the assertion; update both tests to verify the target was not executed before calling start(): use a mutable sentinel or mock as the target (e.g., a function that flips a boolean or increments a counter) and assert the sentinel remains unchanged prior to t.start(), then call t.start(), join or wait as needed, and assert the sentinel changed after execution to confirm proper autostart behavior.test/unittests/test_thread_utils.py-98-106 (1)
98-106:⚠️ Potential issue | 🟠 MajorAssert the timeout exception explicitly in both tests.
Lines 98-106:
assertRaises(Exception)catches any Exception subclass and won't catch wrapper bugs. The decorator raisesException(f'function [{func.__name__}] timeout [{timeout}] exceeded!')on timeout—assert this specific message instead.Lines 137-144: The test only verifies the decorator is callable, not that the default timeout (5 seconds) is enforced. Add a test that a function exceeding the default timeout raises the timeout exception.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_thread_utils.py` around lines 98 - 106, Update the tests to assert the exact timeout exception message from the threaded_timeout decorator instead of a broad Exception: in test_timeout_raises_exception wrap the call to slow_func() with assertRaisesRegex(Exception, r"function \[slow_func\] timeout \[1\] exceeded!") so the exact message from threaded_timeout(timeout=1) is validated; also add a new test (or modify the existing callable-check test around lines 137-144) that defines a function decorated with `@threaded_timeout`() (default timeout 5) which sleeps longer than 5s and asserts the exact message "function [<func_name>] timeout [5] exceeded!" with assertRaisesRegex to verify the default timeout is enforced.test/unittests/test_dialog.py-187-198 (1)
187-198:⚠️ Potential issue | 🟠 MajorDon't let this test pass on arbitrary exceptions.
The broad
except Exception: passmeans a brokenlang=Nonefallback still leaves CI green. This should assert the documented fallback result and only simulate the config import failure you care about.🔧 Tighten the failure mode and add a real assertion
with patch("ovos_utils.dialog.resolve_resource_file", return_value=None): with patch("ovos_utils.dialog.log_deprecation"): - with patch("builtins.__import__", - side_effect=ImportError("no ovos_config")): - # ImportError path — falls back gracefully - try: - result = get_dialog("test", lang=None) - except Exception: - pass # acceptable — the path is exercised + original_import = __import__ + + def _import(name, *args, **kwargs): + if name.startswith("ovos_config"): + raise ImportError("no ovos_config") + return original_import(name, *args, **kwargs) + + with patch("builtins.__import__", side_effect=_import): + result = get_dialog("test", lang=None) + self.assertEqual(result, "test")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_dialog.py` around lines 187 - 198, The test test_get_dialog_none_lang_config_import_error currently swallows all exceptions; change it to only simulate an ImportError for the config import and assert the expected fallback value instead of a blanket except. Specifically, when patching builtins.__import__ (in the test), make the side effect raise ImportError only for the target module (e.g. when the name contains "ovos_config") and otherwise defer to the real __import__; then call get_dialog("test", lang=None) and assert it returns the documented fallback (e.g. the en-us dialog result) instead of using a bare except Exception: pass so only the intended ImportError path is exercised and verified.test/unittests/test_sound.py-25-32 (1)
25-32:⚠️ Potential issue | 🟠 MajorUse scoped patching for the distutils stub instead of module-level
sys.modulesmutation.The stub at lines 25–32 mutates
sys.modulesglobally when the test module is imported, which can override a realdistutilsimplementation if it hasn't been imported yet and will affect all subsequent tests in the same process. This creates test isolation issues and potential order-dependency.The same pattern appears in
test/unittests/test_device_input.py. For both files, useunittest.mock.patch.dict(sys.modules, {...})scoped to the import statement instead, or move the stub into a shared test bootstrap that runs before any module-level imports.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_sound.py` around lines 25 - 32, The test currently mutates sys.modules at import time via the module-level distutils_stub and spawn_stub variables; replace this global mutation with a scoped patch using unittest.mock.patch.dict to inject {"distutils": distutils_stub, "distutils.spawn": spawn_stub} only around the import/initialization that needs it (e.g., wrap the import of the code under test in a with patch.dict(...) context or apply `@patch.dict` on the specific test function), and do the same change in test_device_input.py; keep the same stub names (distutils_stub, spawn_stub, spawn_stub.find_executable) so you can locate and reuse them when converting the module-level sys.modules assignment into a scoped patch.test/unittests/test_bracket_extra.py-159-175 (1)
159-175:⚠️ Potential issue | 🟠 Major
Word("hello")makes both warning checks false positives.Line 163 and Line 172 instantiate
Wordinside the recorded-warning block, andtest_word_emits_warningalready establishes thatWordemitsDeprecationWarning. These tests will still pass ifSentenceorOptionsstop warning. Buildw_objoutside the recorded block, or ignore its warning separately, then assert only on theSentence([w_obj])/Options([w_obj])call.Proposed fix
def test_sentence_emits_warning(self): - with warnings.catch_warnings(record=True) as w: - warnings.simplefilter("always") - from ovos_utils.bracket_expansion import Sentence, Word - w_obj = Word("hello") - from ovos_utils.bracket_expansion import Sentence - Sentence([w_obj]) - self.assertTrue(any(issubclass(warning.category, DeprecationWarning) for warning in w)) + with warnings.catch_warnings(): + warnings.simplefilter("ignore", DeprecationWarning) + from ovos_utils.bracket_expansion import Sentence, Word + w_obj = Word("hello") + with warnings.catch_warnings(record=True) as w: + warnings.simplefilter("always") + Sentence([w_obj]) + self.assertTrue(any(issubclass(warning.category, DeprecationWarning) for warning in w)) def test_options_emits_warning(self): - with warnings.catch_warnings(record=True) as w: - warnings.simplefilter("always") - from ovos_utils.bracket_expansion import Options, Word - w_obj = Word("hello") - from ovos_utils.bracket_expansion import Options - Options([w_obj]) - self.assertTrue(any(issubclass(warning.category, DeprecationWarning) for warning in w)) + with warnings.catch_warnings(): + warnings.simplefilter("ignore", DeprecationWarning) + from ovos_utils.bracket_expansion import Options, Word + w_obj = Word("hello") + with warnings.catch_warnings(record=True) as w: + warnings.simplefilter("always") + Options([w_obj]) + self.assertTrue(any(issubclass(warning.category, DeprecationWarning) for warning in w))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_bracket_extra.py` around lines 159 - 175, The tests test_sentence_emits_warning and test_options_emits_warning are recording warnings that include the DeprecationWarning produced by Word("hello"), creating false positives; fix by creating the Word instance outside the warnings.catch_warnings context (or otherwise suppress/clear its warning) so the recorded-warnings block only wraps the call to Sentence([w_obj]) or Options([w_obj]) respectively, ensuring you assert only on warnings emitted by Sentence and Options (reference: Sentence, Options, Word, test_sentence_emits_warning, test_options_emits_warning)..github/workflows/pip_audit.yml-10-11 (1)
10-11:⚠️ Potential issue | 🟠 MajorPin the reusable workflow to an immutable ref.
Using
@devhere means audit results can change underneath this repo whenever the upstream branch moves. GitHub recommends a commit SHA as the safest reusable-workflow reference, and the risk is amplified bysecrets: inherit. (docs.github.com)🔒 Proposed fix
- uses: OpenVoiceOS/gh-automations/.github/workflows/pip-audit.yml@dev + uses: OpenVoiceOS/gh-automations/.github/workflows/pip-audit.yml@<full-commit-sha>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/pip_audit.yml around lines 10 - 11, The workflow currently references the reusable workflow with a movable ref ("uses: OpenVoiceOS/gh-automations/.github/workflows/pip-audit.yml@dev"); replace the mutable ref with an immutable commit SHA for that upstream repo (i.e., update the uses value to point to the exact commit SHA of OpenVoiceOS/gh-automations you want to pin) so the pip-audit workflow and inherited secrets cannot change unexpectedly; locate the "uses: OpenVoiceOS/gh-automations/.github/workflows/pip-audit.yml@dev" line and swap "@dev" for the chosen commit SHA..github/workflows/license_check.yml-10-11 (1)
10-11:⚠️ Potential issue | 🟠 MajorPin the reusable workflow to an immutable commit SHA.
Using
@devcreates a dependency on a mutable branch reference. GitHub recommends commit SHAs for reusable workflows because they prevent unreviewed changes to the workflow definition. This matters significantly here because secrets are inherited—a compromised or force-pushed upstream branch could alter the job's behavior without your visibility.Proposed fix
- uses: OpenVoiceOS/gh-automations/.github/workflows/license-check.yml@dev + uses: OpenVoiceOS/gh-automations/.github/workflows/license-check.yml@<commit-sha>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/license_check.yml around lines 10 - 11, Replace the mutable branch ref in the reusable workflow invocation — change the "uses: OpenVoiceOS/gh-automations/.github/workflows/license-check.yml@dev" reference to a specific immutable commit SHA for the workflow (i.e., replace "@dev" with "@<commit-sha>"). Update the "uses" line so the workflow imports the exact commit SHA of OpenVoiceOS/gh-automations to ensure secrets: inherit cannot be affected by branch changes; obtain the correct commit SHA from the upstream repo and paste it in place of "@dev"..github/workflows/downstream.yml-11-13 (1)
11-13:⚠️ Potential issue | 🟠 MajorPin this reusable workflow to a commit SHA.
This job calls an external reusable workflow using the mutable
@devtag, creating a supply-chain risk. Upstream changes inOpenVoiceOS/gh-automationscan silently alter this workflow's behavior, especially problematic since it runs automatically on push and daily schedule. Combined withsecrets: inherit, this exposure is significant.Use a pinned commit SHA instead:
uses: OpenVoiceOS/gh-automations/.github/workflows/downstream-check.yml@<commit-sha>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/downstream.yml around lines 11 - 13, The reusable workflow reference in the check_downstream job uses a mutable tag (`@dev`); replace that with a pinned commit SHA to prevent supply-chain changes—update the uses entry for the check_downstream job (the line referencing OpenVoiceOS/gh-automations/.github/workflows/downstream-check.yml@dev) to use a specific commit SHA (OpenVoiceOS/gh-automations/.github/workflows/downstream-check.yml@<commit-sha>) and commit the change; obtain the SHA from the upstream repo and paste it in place of `@dev` before merging..github/workflows/python-support.yml-11-13 (1)
11-13:⚠️ Potential issue | 🟠 MajorPin the reusable workflow to an immutable commit SHA.
@devis a mutable ref in another repository. Combined withsecrets: inherit, changes inOpenVoiceOS/gh-automationscan alter this job's behavior without a PR in this repo. Replace the branch reference with a specific commit SHA to ensure reproducibility and security.Suggested change
- build_tests: - uses: OpenVoiceOS/gh-automations/.github/workflows/build-tests.yml@dev + build_tests: + uses: OpenVoiceOS/gh-automations/.github/workflows/build-tests.yml@<pinned-commit-sha>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/python-support.yml around lines 11 - 13, The job declaration for build_tests currently references the reusable workflow with a mutable ref ("OpenVoiceOS/gh-automations/.github/workflows/build-tests.yml@dev") and uses "secrets: inherit"; update the uses field to pin the reusable workflow to a specific immutable commit SHA (replace "@dev" with the chosen commit hash) to ensure reproducible behavior, keep "secrets: inherit" as required, and then run the workflow/CI to verify the pinned SHA works properly with the build_tests job.test/unittests/test_device_input.py-36-37 (1)
36-37:⚠️ Potential issue | 🟠 MajorPatch the module-local
find_executableconsistently.These decorators patch
distutils.spawn.find_executable, butovos_utils.device_inputimportsfind_executabledirectly into module scope (line 2), creating a module-local binding that is unaffected by patches to the origin. All occurrences must patchovos_utils.device_input.find_executableinstead to prevent tests from reaching the reallibinputandxinputpaths.Affects lines: 36, 44, 64, 83, 94, 102, 118, 137, 165, 193, 213.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_device_input.py` around lines 36 - 37, The tests currently patch distutils.spawn.find_executable but ovos_utils.device_input imported find_executable into module scope, so update all patch decorators to target the module-local symbol (use "ovos_utils.device_input.find_executable") instead of "distutils.spawn.find_executable"; specifically change the patch target on the decorators for the test functions like test_init_no_executables and the other affected tests so they patch ovos_utils.device_input.find_executable to ensure libinput/xinput lookups are stubbed.test/unittests/test_smtp_utils.py-93-106 (1)
93-106:⚠️ Potential issue | 🟠 MajorDon't swallow the config-path failure here.
The
try/except Exception: passmakes this test succeed whenovos_configis unavailable orsend_emailraises before reachingsend_smtp, so the main behavior under test is never actually verified.Proposed test tightening
- with patch("ovos_utils.smtp_utils.LOG"): - try: - with patch("ovos_config.config.read_mycroft_config", - return_value=fake_config): - send_email("Hello", "Body", recipient="recv@test.com") - except Exception: - # If ovos_config is not importable, skip this sub-test - pass - - # If send_smtp was called verify the args - if mock_send_smtp.called: - args, kwargs = mock_send_smtp.call_args - self.assertEqual(args[0], "user@test.com") # user - self.assertEqual(args[2], "user@test.com") # sender + config_module = MagicMock() + config_module.read_mycroft_config.return_value = fake_config + + with patch("ovos_utils.smtp_utils.LOG"), \ + patch.dict("sys.modules", { + "ovos_config": MagicMock(), + "ovos_config.config": config_module, + }): + send_email("Hello", "Body", recipient="recv@test.com") + + mock_send_smtp.assert_called_once_with( + "user@test.com", + "pass123", + "user@test.com", + "recv@test.com", + "Hello", + "Body", + "mail.test.com", + 587, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_smtp_utils.py` around lines 93 - 106, The test currently swallows all exceptions around the send_email call which hides real failures; change the broad try/except to only ignore ImportError from ovos_config by replacing "except Exception: pass" with "except ImportError: pass" (or alternatively remove the try/except and explicitly assert importability of ovos_config before calling send_email), so that other exceptions raised by send_email or test setup (e.g., failures before send_smtp is reached) surface and the subsequent assertions on mock_send_smtp.call_args run; keep references to send_email, mock_send_smtp and ovos_config.config.read_mycroft_config to locate and update the test.ovos_utils/skill_installer.py-316-319 (1)
316-319:⚠️ Potential issue | 🟠 MajorAdd
-yflag touv pip uninstallcommand for non-interactive execution.The
uvpath is missing the-yflag that the pip fallback includes. Without it,uv pip uninstallwill prompt for confirmation, causing the subprocess to hang. Update line 317 to match the pip behavior:pip_args = [self.UV, "pip", "uninstall", "-y"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovos_utils/skill_installer.py` around lines 316 - 319, The pip uninstall invocation omits the non-interactive flag when using the UV wrapper causing subprocess prompts; update the pip_args construction in the code that builds the uninstall command (the branch referencing self.UV and pip_args in skill_installer.py) to include "-y" so it mirrors the fallback path (i.e., make the UV branch use [self.UV, "pip", "uninstall", "-y"]).test/unittests/test_ocp.py-481-498 (1)
481-498:⚠️ Potential issue | 🟠 MajorThis test bakes in the current pointer-reset bug.
After inserting before the selected track, the logical current item should stay selected. The comment on Lines 495-496 normalizes the
position -> 0reset caused byPlaylist.add_entry()validating before the insert, andassertIsInstance(..., int)will never catch that regression.💡 Assert the intended post-insert state
# Insert a new entry at index 0 (before current position) e4 = MediaEntry(uri="http://a.com/4.mp3", title="T4") pl.add_entry(e4, index=0) - # Position was incremented to 3 by add_entry, then validated to 0 by _validate_position - # The key thing is the code path (line 514) was exercised - self.assertIsInstance(pl.position, int) + self.assertEqual(pl.position, 3) + self.assertIs(pl.current_track, e3)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_ocp.py` around lines 481 - 498, The test currently only checks that pl.position is an int and hides the regression where Playlist.add_entry() and Playlist._validate_position() reset the pointer; update the assertions so after calling Playlist.add_entry(e4, index=0) (using Playlist.add_entry and the Playlist.position attribute) you explicitly assert that the logical selection did not change — e.g., that pl.position increased by 1 from the original 2 (so equals 3) and that the entry at pl.position is still the previously selected MediaEntry (e3) or that pl.current (if available) is e3; this ensures the insert-before-current behavior is validated rather than masking the bug.ovos_utils/ocp.py-643-664 (1)
643-664:⚠️ Potential issue | 🟠 Major
MediaTypedrops out ofimport *with this shim.Lines 643-644 delete
MediaTypefrom the module namespace. Sinceovos_utils/ocp.pydoes not define__all__, Python'simport *semantics scanmodule.__dict__directly and will not findMediaType. The property-based re-exposure only works for explicit attribute access (e.g.,from ovos_utils.ocp import MediaTypeorovos_utils.ocp.MediaType), not for wildcard imports.This is a backward-compatibility break. Either add an
__all__list that includes'MediaType', or use a deprecation approach that does not remove it from the module namespace.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovos_utils/ocp.py` around lines 643 - 664, The current shim deletes MediaType from the module namespace which breaks "from module import *"; restore backward compatibility by ensuring MediaType remains discoverable via import *—either add an __all__ list that includes 'MediaType' (e.g., define __all__ = [..., 'MediaType']) or stop deleting MediaType (remove the line "del MediaType") and keep the alias _MediaType plus the property in _DeprecatingModule; update references to the symbols _MediaType, MediaType and _DeprecatingModule accordingly so the deprecation warning still fires while MediaType remains present for wildcard imports.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 92b5e5ae-68bd-40a8-b33a-e72ee37ed83a
⛔ Files ignored due to path filters (2)
test/unittests/log_test/configured.logis excluded by!**/*.logtest/unittests/log_test/rotate.logis excluded by!**/*.log
📒 Files selected for processing (77)
.github/workflows/build-tests.yml.github/workflows/build_tests.yml.github/workflows/coverage.yml.github/workflows/downstream.yml.github/workflows/license_check.yml.github/workflows/lint.yml.github/workflows/pip_audit.yml.github/workflows/publish_stable.yml.github/workflows/python-support.yml.github/workflows/release-preview.yml.github/workflows/release_workflow.yml.github/workflows/repo-health.yml.github/workflows/unit_tests.ymlAUDIT.mdFAQ.mdMAINTENANCE_REPORT.mdQUICK_FACTS.mdSUGGESTIONS.mddocs/events.mddocs/fakebus.mddocs/index.mddocs/log.mddocs/process-utils.mddocs/utilities.mdovos_utils/device_input.pyovos_utils/fakebus.pyovos_utils/file_utils.pyovos_utils/geolocation.pyovos_utils/lang/__init__.pyovos_utils/log_parser.pyovos_utils/network_utils.pyovos_utils/oauth.pyovos_utils/ocp.pyovos_utils/security.pyovos_utils/signal.pyovos_utils/skill_installer.pyovos_utils/skills.pyovos_utils/smtp_utils.pyovos_utils/xml_helper.pypyproject.tomltest/unittests/log_test/rotate.log.1test/unittests/test_bracket_extra.pytest/unittests/test_decorators.pytest/unittests/test_device_input.pytest/unittests/test_dialog.pytest/unittests/test_events.pytest/unittests/test_fakebus.pytest/unittests/test_file_utils_extra.pytest/unittests/test_file_utils_new.pytest/unittests/test_geolocation.pytest/unittests/test_gui.pytest/unittests/test_init.pytest/unittests/test_lang.pytest/unittests/test_list_utils.pytest/unittests/test_log_parser.pytest/unittests/test_messagebus.pytest/unittests/test_metrics.pytest/unittests/test_network_utils_extra.pytest/unittests/test_oauth.pytest/unittests/test_ocp.pytest/unittests/test_ocp_extra.pytest/unittests/test_parse.pytest/unittests/test_process_utils_extra.pytest/unittests/test_security.pytest/unittests/test_security_extra.pytest/unittests/test_signal.pytest/unittests/test_skill_installer.pytest/unittests/test_skills.pytest/unittests/test_smtp_utils.pytest/unittests/test_sound.pytest/unittests/test_ssml_extra.pytest/unittests/test_system.pytest/unittests/test_text_utils.pytest/unittests/test_thread_utils.pytest/unittests/test_version.pytest/unittests/test_xdg_utils.pytest/unittests/test_xml_helper.py
💤 Files with no reviewable changes (2)
- .github/workflows/unit_tests.yml
- .github/workflows/build_tests.yml
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (8)
ovos_utils/thread_utils.py (1)
55-77: Return type annotation inconsistent with docstring.The return type hint at line 56 is
Any, but the docstring at line 67 still stateskthread.KThread. Consider aligning these for consistency.If the intent is to avoid a top-level import for type hints, you could use
TYPE_CHECKING:✨ Suggested improvement for type consistency
from functools import wraps from threading import Thread, Event -from typing import Callable, Optional, Any +from typing import Callable, Optional, Any, TYPE_CHECKING + +if TYPE_CHECKING: + import kthread from ovos_utils.log import LOGThen update the return type:
def create_killable_daemon(target: Callable, args: tuple = (), kwargs: Optional[dict] = None, - autostart: bool = True) -> Any: + autostart: bool = True) -> "kthread.KThread":Alternatively, simply update the docstring to reflect the
Anyreturn type if keeping the simpler approach is preferred.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovos_utils/thread_utils.py` around lines 55 - 77, The function create_killable_daemon has a return annotation of Any but its docstring claims it returns kthread.KThread; make them consistent by either importing kthread for type hints under TYPE_CHECKING and changing the return annotation to kthread.KThread (or "kthread.KThread" as a forward/string type) or by updating the docstring to state it returns Any; reference create_killable_daemon and the kthread.KThread type so the fix is applied to the function signature and its docstring only.test/unittests/test_dialog.py (2)
213-224:join_listassertions are too loose to catch formatting regressions.These checks only prove that a couple of tokens appear somewhere in the output. A broken implementation that reorders items or drops the connector could still pass. Prefer stubbing the localized connector and asserting the exact rendered string.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_dialog.py` around lines 213 - 224, The tests for join_list (test_multiple_items and test_custom_separator) are too loose; replace the current assertIn checks with exact string equality checks by stubbing/mocking the localization lookup that supplies the connector so the connector is deterministic (e.g., mock the internal/localization function used by join_list for lang="en-us" to return "and" or "or"), then call join_list(["a","b","c"], ...) and assert the full rendered string equals the expected "a, b and c" (or with sep=";" expect "a; b; c" or the exact connector placement you expect). Ensure you reference and patch the same localization utility join_list uses so tests reliably assert exact output formatting.
23-108: Add coverage for partial-failure and all-failure template rendering.The new renderer tests are all happy-path cases. The more regression-prone behavior is when some template lines fail to expand versus when every line fails; that distinction is still untested here. Based on learnings: in
ovos_utils/dialog.py, all-line expansion failures should raise, while partial failures should log/drop bad lines and continue.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_dialog.py` around lines 23 - 108, Add two tests to TestMustacheDialogRenderer that exercise failure modes of MustacheDialogRenderer: one where every template line fails to expand and you assert render(...) raises the expected exception from MustacheDialogRenderer.render (all-line expansion failure), and another where only some lines fail so you load a mix of valid and invalid lines via MustacheDialogRenderer.load_template_file and assert render(...) returns a valid expanded line while the invalid lines are dropped (and recent_phrases/max_recent_phrases behavior stays intact); reference MustacheDialogRenderer.render, MustacheDialogRenderer.load_template_file and the recent_phrases/max_recent_phrases attributes to locate code to test.test/unittests/test_ocp.py (2)
15-29: Module-level MediaType import triggers deprecation warning during test collection.The import at line 17 will emit a
DeprecationWarningwhen the test module is loaded. This is acceptable for testing purposes but will cause noise in test output if warnings are not filtered. Consider usingwarnings.filterwarnings("ignore", category=DeprecationWarning, module="ovos_utils.ocp")in test setup, or importing MediaType inside tests that specifically need it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_ocp.py` around lines 15 - 29, The test module imports MediaType at module scope causing a DeprecationWarning during test collection; move the import of MediaType out of the top-level import list in test_ocp.py and either import MediaType inside the specific test(s) that need it or add a warnings.filterwarnings("ignore", category=DeprecationWarning, module="ovos_utils.ocp") in the test setup/fixture so the deprecation warning from ovos_utils.ocp.MediaType is suppressed during collection.
381-401: Consider adding a parallel test for MediaType deprecation warning.This test validates the
available_extractorsdeprecation shim behavior. Given thatMediaTypehas a similar deprecation shim (seeovos_utils/ocp.py:644-680), consider adding a test that verifies theDeprecationWarningis emitted whenMediaTypeis accessed, similar to how this test catches/suppresses warnings foravailable_extractors.📝 Example test for MediaType deprecation
class TestMediaTypeDeprecation(unittest.TestCase): """Test MediaType deprecation shim.""" def test_media_type_emits_deprecation_warning(self) -> None: """Accessing MediaType should emit DeprecationWarning.""" import warnings import importlib import ovos_utils.ocp # Reload to trigger the property access with warnings.catch_warnings(record=True) as w: warnings.simplefilter("always") # Access MediaType to trigger the warning _ = ovos_utils.ocp.MediaType deprecation_warnings = [ x for x in w if issubclass(x.category, DeprecationWarning) ] self.assertTrue( any("ovos_media_classifier" in str(x.message) for x in deprecation_warnings), "Expected deprecation warning mentioning ovos_media_classifier" )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_ocp.py` around lines 381 - 401, Add a new unit test that mirrors TestAvailableExtractors to assert the MediaType deprecation shim emits a DeprecationWarning: create a TestMediaTypeDeprecation class with a test_media_type_emits_deprecation_warning method that uses warnings.catch_warnings(record=True) and warnings.simplefilter("always"), then access ovos_utils.ocp.MediaType to trigger the shim and assert at least one captured warning is a DeprecationWarning and its message mentions "ovos_media_classifier"; reference the MediaType symbol in your test so the import/property access triggers the warning.test/unittests/test_smtp_utils.py (1)
64-74: Test may not exercise the intended code path.This test patches
__import__to raiseImportError, which tests import failure rather than the "missing email config" path. According toovos_utils/smtp_utils.py:59-74, theKeyErroris raised whenconfig.get("email")returns falsy. The current test setup may pass for the wrong reason (import failure) rather than verifying the empty config guard.Consider simplifying to directly mock the config return value:
♻️ Suggested test refactor
def test_send_email_raises_when_no_config(self) -> None: """send_email should raise KeyError when email config is missing.""" from ovos_utils.smtp_utils import send_email - with patch("ovos_utils.smtp_utils.LOG"): - with patch.dict("sys.modules", {"ovos_config": None, - "ovos_config.config": None}): - # Empty config — no email section - with patch("builtins.__import__", side_effect=ImportError): - with self.assertRaises(KeyError): - send_email("subj", "body") + # Mock Configuration to return empty config (no email section) + with patch("ovos_utils.smtp_utils.LOG"), \ + patch("ovos_utils.smtp_utils.Configuration") as mock_cfg: + mock_cfg.return_value.get.return_value = {} # Empty config + with self.assertRaises(KeyError): + send_email("subj", "body")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_smtp_utils.py` around lines 64 - 74, The test test_send_email_raises_when_no_config is currently forcing an ImportError by patching builtins.__import__, which exercises import failure instead of the missing-email-config branch in send_email; update the test to simulate an empty/absent email config by mocking the config lookup used in ovos_utils.smtp_utils (e.g., patch the config.get call used by send_email to return None or {} for the "email" key) and remove the builtins.__import__ side_effect so send_email("subj", "body") raises KeyError for the intended config-path.ovos_utils/skill_installer.py (2)
302-309: Constraint parsing may miss packages with dots or underscores in names.The parsing splits on
~<>=to extract package names, but doesn't handle packages with dots (e.g.,zope.interface) or other edge cases. Also, thereplace("_", "-")normalization only handles underscores, but pip allows both-and_interchangeably in some contexts.This is minor since the constraint file is expected to be well-formed, but consider using a more robust parsing approach if this code path is critical.
♻️ More robust package name extraction
cpkgs = [ - p.split("~")[0] - .split("<")[0] - .split(">")[0] - .split("=")[0] - .replace("_", "-") + re.split(r"[~<>=!;\[]", p)[0].strip().replace("_", "-").lower() for p in cpkgs + if p.strip() and not p.strip().startswith("#") ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovos_utils/skill_installer.py` around lines 302 - 309, The current list comprehension that trims version/operator suffixes from cpkgs is brittle (splitting on "~<>=` and replacing underscores) and can mis-handle names with dots or mixed separators; replace this manual parsing with a robust approach: for each entry in the cpkgs list, parse the requirement using a standards-aware helper (e.g., packaging.requirements.Requirement or pkg_resources.Requirement) to extract the canonical project name and then normalize it with packaging.utils.canonicalize_name (or equivalent) so names with dots, underscores or hyphens are handled correctly; update the code where cpkgs is transformed (the list comprehension shown) to use this parsing path and drop the ad-hoc split/replace logic.
239-249: RuntimeError message may be empty whenprint_logs=True.When
print_logs=True, the subprocess runs withoutstderr=PIPE, soproc.stderrisNone. The guard at line 246-247 prevents a crash, but theRuntimeErrorat line 249 receivesNoneor empty string, providing no diagnostic information.♻️ Provide meaningful error message
if proc.wait() != 0: - stderr = proc.stderr - if stderr: - stderr = stderr.read().decode() + if proc.stderr: + stderr = proc.stderr.read().decode() + else: + stderr = f"pip install failed for {package} (exit code {proc.returncode})" self.play_error_sound() raise RuntimeError(stderr)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovos_utils/skill_installer.py` around lines 239 - 249, When the subprocess started with print_logs=True has a non-zero exit, RuntimeError gets raised with an empty message because proc.stderr is None; update the error handling after proc.wait() in the block around Popen(pip_command) so that if proc.stderr is None you call proc.communicate() (or read proc.stdout if available) to capture any output, and build a helpful message that includes the captured output (or stdout), the pip_command and the exit code (proc.returncode) before calling self.play_error_sound() and raising RuntimeError; reference the existing Popen(pip_command) creation, the print_logs flag, proc.wait()/proc.stderr, proc.communicate(), self.play_error_sound(), and the raised RuntimeError so changes are applied in the correct spot.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.env:
- Around line 1-15: This .env contains shell constructs (#!/bin/sh, if, [
"$OVOS_CHANNEL" = "alpha" ], variable expansion) which break dotenv parsers;
either convert it into a pure dotenv by removing the shebang and conditional and
leaving only KEY=VALUE lines (e.g., OVOS_CHANNEL, PIP_PRE, UV_PRERELEASE,
BASE_URL, PIP_CONSTRAINT, UV_CONSTRAINT) or rename it to a shell script (e.g.,
setup-env.sh), keep the shebang and the if block (using OVOS_CHANNEL, PIP_PRE,
UV_PRERELEASE), and add documentation/README explaining it must be sourced
rather than parsed as a .env file.
In `@test/unittests/test_device_input.py`:
- Around line 23-30: The test currently checks sys.modules to decide whether to
stub distutils, which can shadow a real but not-yet-imported stdlib module;
instead attempt a real import with try: import distutils.spawn (or from
distutils import spawn) and only create and install the
distutils_stub/spawn_stub into sys.modules when that import raises ImportError;
ensure you only inject the stubbed names (distutils and distutils.spawn) when
necessary and keep the stub scope as narrow as possible so it does not leak into
other modules such as ovos_utils.device_input.
In `@test/unittests/test_dialog.py`:
- Around line 178-195: The tests test_get_dialog_none_lang_uses_config and
test_get_dialog_none_lang_config_import_error currently only assert return
values; update them to capture the mocked resolve_resource_file and assert it
was called with a resource path containing the expected language (the configured
language in the first test and "en-us" in the ImportError fallback test).
Specifically, in each test patch resolve_resource_file as a Mock, call
get_dialog("...", lang=None), then inspect resolve_resource_file.call_args or
call_args_list to ensure the path argument includes the correct locale
substring; keep the existing patches for log_deprecation and the sys.modules
tweak for the ImportError case. Ensure assertions reference
resolve_resource_file mock so the tests fail if locale fallback logic is wrong.
- Around line 92-105: The test currently only checks recent_phrases length; make
selection deterministic and assert that recently chosen phrases are avoided by
seeding or stubbing the random selection in MustacheDialogRenderer, then collect
a sequence of rendered outputs and verify no phrase repeats inside a sliding
window of size r.max_recent_phrases (i.e., for each index i ensure outputs[i]
not in outputs[max(0,i-r.max_recent_phrases):i]). Use the renderer methods
referenced (MustacheDialogRenderer, load_template_file, render, recent_phrases,
max_recent_phrases) so the test reliably fails if repetition-avoidance
regresses.
---
Nitpick comments:
In `@ovos_utils/skill_installer.py`:
- Around line 302-309: The current list comprehension that trims
version/operator suffixes from cpkgs is brittle (splitting on "~<>=` and
replacing underscores) and can mis-handle names with dots or mixed separators;
replace this manual parsing with a robust approach: for each entry in the cpkgs
list, parse the requirement using a standards-aware helper (e.g.,
packaging.requirements.Requirement or pkg_resources.Requirement) to extract the
canonical project name and then normalize it with
packaging.utils.canonicalize_name (or equivalent) so names with dots,
underscores or hyphens are handled correctly; update the code where cpkgs is
transformed (the list comprehension shown) to use this parsing path and drop the
ad-hoc split/replace logic.
- Around line 239-249: When the subprocess started with print_logs=True has a
non-zero exit, RuntimeError gets raised with an empty message because
proc.stderr is None; update the error handling after proc.wait() in the block
around Popen(pip_command) so that if proc.stderr is None you call
proc.communicate() (or read proc.stdout if available) to capture any output, and
build a helpful message that includes the captured output (or stdout), the
pip_command and the exit code (proc.returncode) before calling
self.play_error_sound() and raising RuntimeError; reference the existing
Popen(pip_command) creation, the print_logs flag, proc.wait()/proc.stderr,
proc.communicate(), self.play_error_sound(), and the raised RuntimeError so
changes are applied in the correct spot.
In `@ovos_utils/thread_utils.py`:
- Around line 55-77: The function create_killable_daemon has a return annotation
of Any but its docstring claims it returns kthread.KThread; make them consistent
by either importing kthread for type hints under TYPE_CHECKING and changing the
return annotation to kthread.KThread (or "kthread.KThread" as a forward/string
type) or by updating the docstring to state it returns Any; reference
create_killable_daemon and the kthread.KThread type so the fix is applied to the
function signature and its docstring only.
In `@test/unittests/test_dialog.py`:
- Around line 213-224: The tests for join_list (test_multiple_items and
test_custom_separator) are too loose; replace the current assertIn checks with
exact string equality checks by stubbing/mocking the localization lookup that
supplies the connector so the connector is deterministic (e.g., mock the
internal/localization function used by join_list for lang="en-us" to return
"and" or "or"), then call join_list(["a","b","c"], ...) and assert the full
rendered string equals the expected "a, b and c" (or with sep=";" expect "a; b;
c" or the exact connector placement you expect). Ensure you reference and patch
the same localization utility join_list uses so tests reliably assert exact
output formatting.
- Around line 23-108: Add two tests to TestMustacheDialogRenderer that exercise
failure modes of MustacheDialogRenderer: one where every template line fails to
expand and you assert render(...) raises the expected exception from
MustacheDialogRenderer.render (all-line expansion failure), and another where
only some lines fail so you load a mix of valid and invalid lines via
MustacheDialogRenderer.load_template_file and assert render(...) returns a valid
expanded line while the invalid lines are dropped (and
recent_phrases/max_recent_phrases behavior stays intact); reference
MustacheDialogRenderer.render, MustacheDialogRenderer.load_template_file and the
recent_phrases/max_recent_phrases attributes to locate code to test.
In `@test/unittests/test_ocp.py`:
- Around line 15-29: The test module imports MediaType at module scope causing a
DeprecationWarning during test collection; move the import of MediaType out of
the top-level import list in test_ocp.py and either import MediaType inside the
specific test(s) that need it or add a warnings.filterwarnings("ignore",
category=DeprecationWarning, module="ovos_utils.ocp") in the test setup/fixture
so the deprecation warning from ovos_utils.ocp.MediaType is suppressed during
collection.
- Around line 381-401: Add a new unit test that mirrors TestAvailableExtractors
to assert the MediaType deprecation shim emits a DeprecationWarning: create a
TestMediaTypeDeprecation class with a test_media_type_emits_deprecation_warning
method that uses warnings.catch_warnings(record=True) and
warnings.simplefilter("always"), then access ovos_utils.ocp.MediaType to trigger
the shim and assert at least one captured warning is a DeprecationWarning and
its message mentions "ovos_media_classifier"; reference the MediaType symbol in
your test so the import/property access triggers the warning.
In `@test/unittests/test_smtp_utils.py`:
- Around line 64-74: The test test_send_email_raises_when_no_config is currently
forcing an ImportError by patching builtins.__import__, which exercises import
failure instead of the missing-email-config branch in send_email; update the
test to simulate an empty/absent email config by mocking the config lookup used
in ovos_utils.smtp_utils (e.g., patch the config.get call used by send_email to
return None or {} for the "email" key) and remove the builtins.__import__
side_effect so send_email("subj", "body") raises KeyError for the intended
config-path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b305a1de-4a97-4e2b-997c-acf4c6750562
⛔ Files ignored due to path filters (3)
test/unittests/log_test/configured.logis excluded by!**/*.logtest/unittests/log_test/rotate.logis excluded by!**/*.loguv.lockis excluded by!**/*.lock
📒 Files selected for processing (17)
.envAUDIT.mdFAQ.mdMAINTENANCE_REPORT.mdMANIFEST.inovos_logs_console_scriptovos_utils/ocp.pyovos_utils/skill_installer.pyovos_utils/thread_utils.pyovos_utils/version.pysetup.pytest/unittests/log_test/rotate.log.1test/unittests/test_bracket_extra.pytest/unittests/test_device_input.pytest/unittests/test_dialog.pytest/unittests/test_ocp.pytest/unittests/test_smtp_utils.py
💤 Files with no reviewable changes (2)
- MANIFEST.in
- setup.py
✅ Files skipped from review due to trivial changes (1)
- FAQ.md
🚧 Files skipped from review as they are similar to previous changes (2)
- test/unittests/log_test/rotate.log.1
- test/unittests/test_bracket_extra.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@AUDIT.md`:
- Around line 127-136: Update AUDIT.md to mark the two findings as resolved or
move them into a historical subsection: the "pyproject.toml — Missing
`python-dateutil` dependency" entry should be annotated as resolved (reference
that `pyproject.toml` now includes `python-dateutil`), and the
"ovos_utils/log.py:405 — `FileNotFoundError` in `get_available_logs`" entry
should be annotated as resolved or moved to historical with a note pointing to
the MAINTENANCE_REPORT.md entry that says `ovos_utils/log.py` was updated to
guard missing log directories; ensure the headings match exactly so future
searches for those symbols find the resolution.
- Around line 66-75: Remove or update the outdated AUDIT.md entry that claims
ovos_utils/geolocation.py uses a falsy check for lat/lon; the note is incorrect
(non-empty strings like "0" are truthy) and the actual code already uses the
correct check (if lat is not None and lon is not None). Edit the AUDIT.md item
to either delete this entry or replace it with a brief note stating the bug was
fixed and referencing the current condition in ovos_utils/geolocation.py (the
lat/lon None-check) for clarity.
In `@MAINTENANCE_REPORT.md`:
- Around line 35-57: The heading "## [2026-03-11] — AUDIT.md bug fixes (13
confirmed bugs)" is inaccurate; either update the parenthetical count to match
the actual number of bullet items in the Changes list or consolidate related
bullets under grouped entries; locate the exact heading string and replace "(13
confirmed bugs)" with the correct count (or refactor the bullet list into
grouped entries and adjust the heading accordingly) so the heading number
matches the body.
In `@ovos_utils/ocp.py`:
- Around line 519-521: The current insert logic bumps the position
unconditionally when index <= self.position, which causes the first insert into
an empty playlist to call set_position(1) and trigger _validate_position()
logging an invalid-pointer; change the guard to only bump on inserts when the
playlist already has items (e.g., require len(self) > 0 or self.position != 0 in
the condition). Update the block around self.insert(index, entry) so it only
calls self.set_position(self.position + 1) if index <= self.position and the
playlist was non-empty before the insert, referencing the insert method, the
position attribute, set_position, and _validate_position.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bd6ff982-7f3b-4a71-a97a-0979475bbc30
📒 Files selected for processing (8)
.github/workflows/build-tests.ymlAUDIT.mdMAINTENANCE_REPORT.mdovos_utils/log.pyovos_utils/ocp.pyovos_utils/signal.pypyproject.tomltest/unittests/test_signal.py
💤 Files with no reviewable changes (2)
- test/unittests/test_signal.py
- ovos_utils/signal.py
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/build-tests.yml
JarbasAl
changed the title
- security.py:64-67: open(cert_path, "wt") → "wb"; dump_certificate and dump_privatekey return bytes, not str — writing in text mode raises TypeError at runtime (CodeRabbit critical) - log_parser.py: flush pending traceback at EOF; if file ends while trace is non-None the final traceback was silently dropped from parse_file output (CodeRabbit major, outside diff) - ocp.py:519-521: guard position bump with had_current_track; first insert into empty playlist was calling set_position(1) which tripped _validate_position() and logged a spurious invalid-pointer error (CodeRabbit minor) - test_security_extra.py: update mock return values to bytes to match the now-correct binary write mode - AUDIT.md: mark geolocation lat/lon falsy-check, python-dateutil dep, and log.py listdir entries as resolved (fixes were already in PR) - MAINTENANCE_REPORT.md: correct bug-count in section heading from "13 confirmed bugs" to accurate "20 confirmed fixes across 10 files" Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
JarbasAl
added a commit
that referenced
this pull request
Mar 11, 2026
* Update downstream dependencies for ovos-utils * fix: timezone woes (#343) * fix: timezone woes migrate bus api client to ovos-bus-client package * fix: tests workflow * fix: update event message caught by coderabbit, been broken since mycroft times... * fix: bad test why do we test if we are just testing the bad behaviour... * Increment Version to 0.8.5a1 * Update Changelog * Update downstream dependencies for ovos-utils * Update downstream dependencies for ovos-utils * Update downstream dependencies for ovos-utils * Update downstream dependencies for ovos-utils * Update downstream dependencies for ovos-utils * Update downstream dependencies for ovos-utils * Update downstream dependencies for ovos-utils * Update downstream dependencies for ovos-utils * Add renovate.json (#347) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * Increment Version to 0.8.5a2 * Update Changelog * chore(deps): update dependency python to 3.14 (#348) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * Increment Version to 0.8.5a3 * Update Changelog * fix: prevent handler errors from aborting FakeBus emit (#358) * fix: better error handling * fix: workflows * Increment Version to 0.8.6a1 * Update Changelog * chore(deps): update actions/setup-python action to v6 (#353) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * chore(deps): update actions/checkout action to v6 (#352) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * Increment Version to 0.8.6a2 * Update Changelog * Delete .github/workflows/coverage.yml * fix: make the stopwatch test less strict (#360) Right now it depends very much on the machine running the test being quick enough, and at least the Alpine CI builders don't seem to be quick enough. Making it less strict still confirms functionality but allows slower machines to also pass the tests. * chore: Add comprehensive test suites and documentation (#362) * chore: update CI/CD workflows to gh-automations@dev standard - Replace inline build/publish/coverage jobs with reusable gh-automations workflows - Add missing workflows: build-tests, lint, license_check, pip_audit, release-preview, repo-health, python-support - Migrate from TigreGotico/gh-automations@master to OpenVoiceOS/gh-automations@dev - Replace codecov-based coverage.yml with coverage.yml@dev reusable workflow - Migrate downstream.yml from inline pipdeptree script to downstream-check.yml@dev - Remove deprecated build_tests.yml and unit_tests.yml (replaced by build-tests.yml) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * test: add 178 new tests, raise coverage from 25% to 29% New test files: - test_init.py: json_dumps, json_loads, datestr2ts (deprecated) - test_text_utils.py: camel_case_split, collapse_whitespaces, rm_parentheses, remove_accents_and_punct - test_list_utils.py: rotate_list, flatten_list, deduplicate_list - test_decorators.py: classproperty, timed_lru_cache (with/without args, expiry, cache_clear) - test_ocp.py: all enums, MediaEntry (CRUD, update, as_dict, from_dict, infocard, mimetype), PluginStream, Playlist (add/remove/sort/replace/navigate) - test_fakebus.py: FakeMessage (serialize/deserialize, forward/reply/response/publish), FakeBus (emit, on/once/remove, wait_for_message/response, run_forever) - test_ssml_extra.py: full SSMLBuilder coverage (sub, emphasis, parts_of_speech, pause, audio, prosody, pitch, volume, rate, phoneme, voice, whisper, remove_ssml, extract_ssml_tags) Module coverage improvements: - ssml.py: 67% → 94% - decorators.py: 0% → 74% - list_utils.py: 40% → 73% - ocp.py: 15% → 26% - fakebus.py: 26% → 41% Also fix pyproject.toml: change license = "Apache" to license = {text = "Apache-2.0"} to comply with PEP 621 / setuptools validation. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix: correct 13 bugs found in full audit - oauth.py: inverted >= expiry check caused token refresh on every call; missing None guard before expires_at; expires_in read from stale pre-refresh dict instead of new_token_data - network_utils.py: operator precedence bug — cfg.get("key" or fallback) always passed first key; bare except swallowing KeyboardInterrupt - skill_installer.py: proc.stderr.read() unguarded when stderr is None - lang/__init__.py: split("-", 2) unpacked into 2 vars crashed on 3-part BCP-47 tags (e.g. zh-Hant-TW); bare excepts fixed - log_parser.py: rstrip result discarded (newlines never stripped); @classmethod used self instead of cls; open('w') truncated log file when testing writability (changed to 'a'); bare excepts fixed - geolocation.py: if lat and lon treated 0.0 as falsy, skipping valid equator/meridian coordinates - file_utils.py: _changed_files list → set for O(1) membership; bare except - fakebus.py: bare except: → except Exception: (3 occurrences) - device_input.py: bare except: → except Exception: - xml_helper.py: bare except: → except Exception: - security.py: unclosed file handles replaced with with-statements - signal.py: type annotations added to all public deprecated functions - skills.py: get_non_properties return type Set[str] added - smtp_utils.py: full parameter and return type annotations added - ocp.py: typing improvements throughout Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * test: raise coverage from 55% to 85% Expanded existing test stubs and added new test files across all major modules. Key additions: - test_oauth.py: full token lifecycle, expiry, refresh, and error paths - test_geolocation.py: mocked requests for all geocoding functions - test_log_parser.py: parsing, filtering, classmethod, writability tests - test_sound.py: sys.modules stubs for pygame/distutils (Python 3.13+) - test_device_input.py: evdev/xinput mocked; 100% coverage achieved - test_signal.py: deprecated module exercised via importlib reload - test_smtp_utils.py: SMTP_SSL mocked end-to-end - test_system.py: subprocess mocked for all system utility paths - test_gui.py: FakeBus-backed GUI helper tests - test_security_extra.py, test_process_utils_extra.py, test_network_utils_extra.py, test_file_utils_new.py: gap-filling - test_thread_utils.py, test_version.py, test_skills.py: new files All test methods carry -> None annotations and docstrings per standards. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * docs: add full audit findings and repo documentation - AUDIT.md: complete file:line-cited findings — critical bugs, performance issues, type annotation gaps, and code smells from full source audit - SUGGESTIONS.md: architecture-level refactor proposals (log_parser CLI split, security module modernisation, etc.) - FAQ.md: keyword-rich Q&A covering common usage questions - QUICK_FACTS.md: machine-readable package reference (version, key classes, entry points) - MAINTENANCE_REPORT.md: AI-transparency log of all changes made - docs/index.md: repo overview and navigation Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * Fix build attribute error and address audit findings * Fix missing dependencies and CI workflow collisions * Fix FileNotFoundError in log utils when log directory is missing * update * update * update * Address CodeRabbit audit feedback and improve test suite * fix: address CodeRabbit review comments - security.py:64-67: open(cert_path, "wt") → "wb"; dump_certificate and dump_privatekey return bytes, not str — writing in text mode raises TypeError at runtime (CodeRabbit critical) - log_parser.py: flush pending traceback at EOF; if file ends while trace is non-None the final traceback was silently dropped from parse_file output (CodeRabbit major, outside diff) - ocp.py:519-521: guard position bump with had_current_track; first insert into empty playlist was calling set_position(1) which tripped _validate_position() and logged a spurious invalid-pointer error (CodeRabbit minor) - test_security_extra.py: update mock return values to bytes to match the now-correct binary write mode - AUDIT.md: mark geolocation lat/lon falsy-check, python-dateutil dep, and log.py listdir entries as resolved (fixes were already in PR) - MAINTENANCE_REPORT.md: correct bug-count in section heading from "13 confirmed bugs" to accurate "20 confirmed fixes across 10 files" Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * update --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> * Increment Version to 0.8.5a4 * Update Changelog --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: JarbasAI <33701864+JarbasAl@users.noreply.github.com> Co-authored-by: JarbasAl <JarbasAl@users.noreply.github.com> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Bart Ribbers <bribbers@disroot.org> Co-authored-by: Claude Sonnet 4.6 <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
New Features
Bug Fixes
Documentation
Refactor
Tests