Merged
Conversation
…ipeline_available() - Add PADACIOSO_PIPELINE constant (pure Python, always available via ovos-workshop) - Add M2V_PIPELINE constant for vector-based intent matching - Update DEFAULT_TEST_PIPELINE to include padacioso stages alongside padatious (padacioso is always available; padatious is optional and may not be on CI) - Add MiniCroft._check_pipeline_available(): raises RuntimeError early if any stage in default_pipeline is not installed — fails loudly instead of silently producing fewer messages than expected - Add is_pipeline_available(pipeline): cheap entry-point check usable in setUpClass to skip tests when optional pipeline plugins are absent Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…overrides Add `pipeline_config: Optional[Dict[str, Dict]] = None` to `MiniCroft.__init__` so tests can override pipeline plugin configuration (e.g. M2V model path) without relying on the user's local mycroft.conf. Overrides are patched into `Configuration()["intents"]` before `super().__init__()` and restored in `stop()`. Useful for forcing the multilingual M2V model in tests regardless of what mycroft.conf has configured locally (e.g. a language-specific model). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughAdds MiniCroft Changes
Sequence DiagramsequenceDiagram
participant Test
participant MiniCroft
participant Configuration
participant Plugins
Test->>MiniCroft: __init__(pipeline_config)
MiniCroft->>MiniCroft: record _original_pipeline_configs / _had_pipeline_configs
MiniCroft->>Configuration: patch Configuration()["intents"] with pipeline_config
MiniCroft->>Plugins: super().__init__() (plugin initialization)
Plugins->>Configuration: read intents config (sees overrides)
Plugins->>Plugins: initialize using overridden config
Test->>MiniCroft: run()/stop()
MiniCroft->>Configuration: restore original Configuration()["intents"] (or remove added keys)
MiniCroft->>Test: stop complete
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Beep boop! Data processing finished. ⚙️I've aggregated the results of the automated checks for this PR below. 🏷️ Release PreviewA sneak peek into the future! 🔮 Current:
✅ PR title follows conventional commit format. 🚀 Release Channel Compatibility Predicted next version:
📋 Repo HealthI've performed a holistic audit of your project's soul. 🧘 ✅ All required files present. Latest Version: ✅ ⚖️ License CheckChecking for any missing license headers. ✍️ ✅ No license violations found (60 packages). License distribution: 14× MIT, 14× MIT License, 9× Apache Software License, 4× Apache-2.0, 4× BSD-3-Clause, 2× ISC License (ISCL), 2× PSF-2.0, 2× Python Software Foundation License, +8 more Full breakdown — 60 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. 🔒 Security (pip-audit)Shields up! Scanning for potential threats. 🛡️ ✅ No known vulnerabilities found (68 packages scanned). 🔨 Build TestsChecking if the code is properly tempered. ⚔️ ✅ All versions pass
Beep boop, I'm just a script 🤖 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
ovoscope/__init__.py (1)
222-252: Consider extracting the suffix-stripping logic into a helper.The suffix-stripping loop (lines 237-241) is duplicated in
is_pipeline_available()(lines 419-422). Extracting this to a small helper would improve maintainability.♻️ Proposed refactor to extract helper
+def _strip_priority_suffix(stage: str) -> str: + """Strip -high/-medium/-low suffix from a pipeline stage ID.""" + for suffix in ("-high", "-medium", "-low"): + if stage.endswith(suffix): + return stage[: -len(suffix)] + return stage + + class MiniCroft(SkillManager): # ... def _check_pipeline_available(self, pipeline: List[str]) -> None: # ... for stage in pipeline: - # Strip priority suffix to get the base plugin ID - base = stage - for suffix in ("-high", "-medium", "-low"): - if stage.endswith(suffix): - base = stage[: -len(suffix)] - break + base = _strip_priority_suffix(stage) if base not in available: missing.append(stage)Apply the same change to
is_pipeline_available().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovoscope/__init__.py` around lines 222 - 252, Extract the repeated suffix-stripping logic into a small helper (e.g., _strip_priority_suffix or _normalize_stage_id) that takes a stage string and returns the base plugin id after removing "-high"/"-medium"/"-low"; then replace the loop in _check_pipeline_available and the similar code in is_pipeline_available with calls to this helper, update imports/uses accordingly, and ensure tests still use the same behavior and return values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@ovoscope/__init__.py`:
- Around line 222-252: Extract the repeated suffix-stripping logic into a small
helper (e.g., _strip_priority_suffix or _normalize_stage_id) that takes a stage
string and returns the base plugin id after removing "-high"/"-medium"/"-low";
then replace the loop in _check_pipeline_available and the similar code in
is_pipeline_available with calls to this helper, update imports/uses
accordingly, and ensure tests still use the same behavior and return values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a82ce7f7-5109-4049-bd28-44154e5f4803
📒 Files selected for processing (4)
FAQ.mdMAINTENANCE_REPORT.mddocs/minicroft.mdovoscope/__init__.py
Cover: config patched while running, restored after stop(), existing key preserved on restore, None does not patch, multiple keys all patched/restored. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Introduces `MiniListener`, `get_mini_listener()`, and `ListenerTest` to enable end-to-end testing of AudioTransformer plugins through the full `AudioTransformersService` pipeline on a FakeBus — without real hardware. - `ovoscope/listener.py` — new module with MiniListener, get_mini_listener, ListenerTest dataclass - `ovoscope/__init__.py` — re-export new symbols via try/except import - `docs/listener.md` — new: MiniListener API reference with source citations - `docs/index.md` — add listener section, update Public API table Validated with GGWavePlugin as first concrete test (see ggwave plugin repo). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds listen(audio, language, stt_instance) to MiniListener — the first
method that exercises the complete listener pipeline end-to-end:
WAV file / PCM bytes
→ AudioTransformersService.transform()
→ stt_instance.execute(AudioData, language)
→ recognizer_loop:utterance (FakeBus)
→ captured messages
Also adds _wav_to_audio_data() helper that uses AudioData.from_file()
for file paths and the wave stdlib module for raw bytes, correctly
reading sample_rate and sample_width from the WAV header.
Removes MiniSTT/stt.py (wrong abstraction level — STT is part of the
listener pipeline, not a separate harness).
Validated by 17 ROVER STT E2E tests that feed the real jfk.wav through
MiniListener.listen() with mocked backends (ROVER / wROVER / itROVER).
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add full pipeline conceptual diagram (audio → transformers → STT → utterance) - Document MiniListener.listen() signature, sequence, and _wav_to_audio_data helper - Add STT pipeline quick-start example - Add cross-reference to ROVER E2E test as second concrete usage example - Correct source line citations for MiniListener Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…o test utilities Introduces `ovoscope.audio` — a new optional module for testing ovos-audio services without real hardware, TTS engines, or network access. New public API: - `MockAudioBackend` — no-op AudioBackend tracking play/pause/stop/duck state - `AudioServiceHarness` — context manager wrapping AudioService + MockAudioBackend on a FakeBus; now registers all bus handlers including get/set_track_position, get_track_length, seek_forward, seek_backward - `MockTTS` — no-op TTS writing a silent WAV, recording spoken_utterances - `PlaybackServiceHarness` — context manager wrapping PlaybackService + MockTTS - `AudioCaptureSession` — records bus messages by prefix for sequence assertions - `SILENT_WAV` — minimal valid WAV bytes constant for instant-play tests Also adds: - `pyproject.toml` optional dependency `[audio]` extra - `docs/audio-testing.md` — full API reference with examples - `test/unittests/test_audio_harness.py` — 38 unit tests for all harness classes - FAQ.md / QUICK_FACTS.md / AUDIT.md updated Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
ovoscope/listener.py (1)
310-360: Avoid exposing a silent no-opstt_instanceparameter.Passing
stt_instanceintoget_mini_listener()currently has no effect, so it's easy to think STT was configured when it was not. For a new public API, either remove the argument now or persist it onMiniListenerand use it as the default inlisten().Possible direction
class MiniListener: def __init__( self, config: Dict[str, Any], plugin_instances: Optional[Dict[str, Any]] = None, + stt_instance: Optional[Any] = None, ) -> None: @@ self._messages: List[Message] = [] + self._default_stt_instance = stt_instance @@ def listen( self, audio: Union[bytes, str, Path], language: str = "en-us", stt_instance: Optional[Any] = None, @@ - if stt_instance is not None: + stt_instance = stt_instance or self._default_stt_instance + if stt_instance is not None: @@ def get_mini_listener( @@ ) -> MiniListener: @@ - return MiniListener(config, plugin_instances=plugin_instances) + return MiniListener( + config, + plugin_instances=plugin_instances, + stt_instance=stt_instance, + )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovoscope/listener.py` around lines 310 - 360, The get_mini_listener() exposes an unused stt_instance which is misleading; persist it on the created MiniListener instead of silently ignoring it: update get_mini_listener to pass stt_instance through to MiniListener, add a constructor/attribute on class MiniListener (e.g. self._stt_instance) to store it, and change MiniListener.listen(self, ..., stt_instance=None) to use the stored self._stt_instance as the default when stt_instance is None so callers that omit stt_instance get the injected instance.
🤖 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/index.md`:
- Around line 87-94: Update the documentation text that claims MiniListener does
not cover STT to reflect the new behavior: change the wording that says
"MiniListener does not cover STT" (referenced near the block describing
MiniListener and the listener/STT pipeline) to state that MiniListener.listen()
now includes the STT pipeline or remove the contradictory sentence entirely;
ensure the section describing MiniListener and the "Listener Pipeline Testing"
paragraph aligns with the MiniListener.listen() flow and mentions STT support
where appropriate.
In `@MAINTENANCE_REPORT.md`:
- Around line 6-20: The report contains duplicated "ovoscope/" prefixes in
several paths; update the entries so they point to the real locations by
removing the extra prefix (e.g. change "ovoscope/ovoscope/audio.py" ->
"ovoscope/audio.py", "ovoscope/ovoscope/__init__.py" -> "ovoscope/__init__.py",
and drop the extra "ovoscope/" in docs entries like
"ovoscope/docs/audio-testing.md", "ovoscope/docs/index.md" and
"ovoscope/FAQ.md"); apply the same fix to the other affected entries referenced
around lines 33-38 so all paths consistently match the repository layout used in
the PR.
In `@ovoscope/__init__.py`:
- Around line 862-880: The current blanket "except ImportError: pass" hides any
import-time errors in ovoscope.audio and ovoscope.listener; change each block to
catch ImportError as e and only silence it if it indicates the optional
dependency itself is missing (e.g., check e.name or isinstance(e,
ModuleNotFoundError) and that the missing module matches the optional backend),
otherwise re-raise the exception so real regressions in ovoscope.audio or
ovoscope.listener surface; update the two try/except blocks around the imports
from ovoscope.audio (MockAudioBackend, AudioServiceHarness, MockTTS,
PlaybackServiceHarness, AudioCaptureSession) and ovoscope.listener
(MiniListener, get_mini_listener, ListenerTest) accordingly.
- Around line 170-183: You patch the global Configuration() with
self._pipeline_config before completing object startup which can leak when
startup aborts; to fix, ensure the pre-init changes to intents_cfg are either
applied only after successful boot or reverted on failure: wrap the remainder of
initialization/startup (the call that leads to run() in get_minicroft()/__init__
and any call that may raise) in a try/except/finally and on exception restore
intents_cfg using the recorded self._had_pipeline_configs and
self._original_pipeline_configs (or only set intents_cfg[plugin_key] if had been
present, otherwise delete it) so pipeline overrides are rolled back if
initialization does not complete, or alternatively move the patch to after
successful start so stop() won't be skipped.
- Around line 82-97: DEFAULT_TEST_PIPELINE currently includes padatious stages
("ovos-padatious-pipeline-plugin-*") which causes get_minicroft() to fail when
padatious/SWIG isn't installed; remove all padatious entries from
DEFAULT_TEST_PIPELINE and leave only padacioso/other plugins, and ensure any
callers that require padatious explicitly opt into PADATIOUS_PIPELINE; update
references to DEFAULT_TEST_PIPELINE and any logic in get_minicroft() that
assumes padatious is present so it no longer fails fast when padatious stages
are missing.
In `@test/unittests/test_minicroft.py`:
- Around line 220-320: Update all get_minicroft(...) invocations in these tests
to pass default_pipeline=None so tests don't boot the full default pipeline;
e.g., for calls inside test_pipeline_config_patches_intents_config,
test_pipeline_config_restored_after_stop,
test_pipeline_config_preserves_existing_key,
test_pipeline_config_none_does_not_patch, and test_pipeline_config_multiple_keys
add default_pipeline=None to the get_minicroft(...) argument list to decouple
the tests from default pipeline plugins and ensure only pipeline_config
patch/restore behavior is exercised.
---
Nitpick comments:
In `@ovoscope/listener.py`:
- Around line 310-360: The get_mini_listener() exposes an unused stt_instance
which is misleading; persist it on the created MiniListener instead of silently
ignoring it: update get_mini_listener to pass stt_instance through to
MiniListener, add a constructor/attribute on class MiniListener (e.g.
self._stt_instance) to store it, and change MiniListener.listen(self, ...,
stt_instance=None) to use the stored self._stt_instance as the default when
stt_instance is None so callers that omit stt_instance get the injected
instance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8a6aeeaa-cbff-4b3b-96a7-16fba8392b14
📒 Files selected for processing (6)
MAINTENANCE_REPORT.mddocs/index.mddocs/listener.mdovoscope/__init__.pyovoscope/listener.pytest/unittests/test_minicroft.py
✅ Files skipped from review due to trivial changes (1)
- docs/listener.md
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 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 14-24: The audit section is inconsistent: it omits the newly added
test file test/unittests/test_audio_harness.py and still refers to
ovoscope/setup.py as declaring license even though setup.py was removed in this
diff; update the audit text to list test/unittests/test_audio_harness.py among
the unit tests and remove or correct any claims about ovoscope/setup.py (replace
with the actual current license location or note absence of LICENSE file),
ensuring the "Missing LICENSE file" entry references the correct file or repo
state so the section is internally consistent.
In `@docs/audio-testing.md`:
- Around line 16-17: Update the broken docs references that point to
ovoscope/ovoscope/audio.py (e.g. the reference for AudioServiceHarness) to the
correct module path ovoscope/audio.py and remove hard-coded line numbers;
instead use symbol-based anchors or Sphinx/Markdown cross-references (e.g., link
to the AudioServiceHarness class/function by name) so future edits won't break
the links—search for all occurrences that reference ovoscope/ovoscope/audio.py
(lines called out: 16-17, 36-37, 103, 109, 124, 143, 154, 167) and replace them
with stable references to ovoscope/audio.py and the AudioServiceHarness symbol
or use documentation tooling anchors.
- Around line 23-32: The examples reference undefined names so they fail when
copy-pasted; update each snippet to include the missing imports and consistent
harness variable usage: add an import for Message (or the correct message class
used by your bus) and for time where used, and ensure the harness is created
with "with AudioServiceHarness() as h" (or rename the harness variable
consistently) so calls like h.play, h.bus.emit, time.sleep, h.assert_playing and
h.assert_volume_lowered all resolve; adjust the three example blocks to include
these imports and the with-statement so they are runnable as-is.
- Around line 3-4: Add a short prerequisite note at the top of the document
explaining that the audio harnesses live behind the optional "audio" extra and
must be installed before importing ovoscope.audio; mention
ovos-audio/ovoscope.audio explicitly and provide an example install command
(e.g., pip install <project-package>[audio]) so readers know to install the
audio extra to avoid import failures when using ovoscope.audio.
In `@ovoscope/audio.py`:
- Around line 568-591: The patcher self._play_audio_patcher is started before
the fallible setup (PlaybackService(...) and self.mock_tts.init(...)), so if
either raises the started patch leaks into other tests; wrap the remainder of
__enter__() in a try/finally (or use contextlib.ExitStack) and in the finally
call self._play_audio_patcher.stop() when an exception occurs, then re-raise the
exception so partial initialization does not leave the global patch active;
ensure this covers the PlaybackService construction and self.mock_tts.init(...)
sections.
- Around line 606-624: The speak method currently ignores the boolean result of
self._audio_output_end.wait(timeout) so it returns even when playback never
finished; update speak (in the speak method) to capture the wait result and
raise a timeout exception (e.g., TimeoutError or a project-specific exception)
if the wait returns False, making sure callers receive an error when
recognizer_loop:audio_output_end does not arrive; preserve the existing clearing
of _audio_output_start/_audio_output_end/_mic_listen and include context in the
raised error (utterance and timeout) for debugging.
In `@test/unittests/test_audio_harness.py`:
- Around line 33-40: The test unconditionally imports ovoscope.audio (and
symbols AudioCaptureSession, AudioServiceHarness, MockAudioBackend, MockTTS,
PlaybackServiceHarness, SILENT_WAV) but the "audio" extra may not be installed
in CI; add a guard so tests are skipped when the audio extra is unavailable.
Either wrap the import in a try/except ImportError and set AUDIO_AVAILABLE =
False (then decorate the test class or test functions with
unittest.skipUnless(AUDIO_AVAILABLE, "audio extra not installed")), or add a
top-level
unittest.skipUnless(importlib.util.find_spec("ovos_plugin_manager.templates.audio"),
"audio extra not installed") to the module so collection doesn't fail.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1ebf2e8d-43c7-4f2e-baea-02f4e04e2ac0
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (7)
AUDIT.mdFAQ.mdQUICK_FACTS.mddocs/audio-testing.mdovoscope/audio.pypyproject.tomltest/unittests/test_audio_harness.py
✅ Files skipped from review due to trivial changes (2)
- QUICK_FACTS.md
- FAQ.md
- Add [dev] extra to pyproject.toml including audio and pydantic dependencies - Update CI workflows to use Python 3.12 and install [dev] extras - Add @unittest.skipUnless(AUDIO_AVAILABLE) guards to all audio test classes - Fix python_version matrix in build and release workflows
- Fix references to ovoscope/ovoscope/audio.py -> ovoscope/audio.py - Remove hardcoded line numbers from documentation and audit reports - Update AUDIT.md to reflect fixed status of unit tests
There was a problem hiding this comment.
🧹 Nitpick comments (2)
test/unittests/test_audio_harness.py (2)
205-210: Assert queued tracks before leaving the harness context.Line 210 checks backend state after
__exit__, so this test also depends on teardown preserving playback state. Keeping the assertion inside thewithblock makes it validate only the queue behavior.Suggested change
def test_queue_appends_to_played_tracks(self) -> None: """queue() must append extra tracks to backend.played_tracks.""" with AudioServiceHarness() as h: h.play(["http://example.com/song1.mp3"]) h.queue(["http://example.com/song2.mp3"]) - self.assertIn("http://example.com/song2.mp3", h.backend.played_tracks) + self.assertIn("http://example.com/song2.mp3", h.backend.played_tracks)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_audio_harness.py` around lines 205 - 210, The assertion currently checks h.backend.played_tracks after the AudioServiceHarness context has exited; move the assertion into the with block so the test validates queue behavior before teardown. Specifically, inside test_queue_appends_to_played_tracks keep using AudioServiceHarness() as h, call h.play(...) and h.queue(...), then assert "http://example.com/song2.mp3" in h.backend.played_tracks while still inside the with block (before __exit__ runs) to avoid relying on teardown to preserve playback state.
169-188: Replace fixed sleeps with explicit waits or mocked time.These tests currently depend on real-time delays, including two 1.1s sleeps. That makes the suite slower and more brittle under CI load. For bus emissions, prefer a
threading.Event/bounded polling helper; for the stop-guard cases, patch the clock or move the guard state directly instead of sleeping past it.Also applies to: 212-250, 383-418
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_audio_harness.py` around lines 169 - 188, Replace real-time sleeps with deterministic waits/mocks: in test_stop_after_guard_clears_playing set or patch the AudioService instance's play_start_time (access via AudioServiceHarness()._service or similar) to a past timestamp so the 1‑second guard is bypassed instead of time.sleep(1.1), then call h.stop() and assert stopped; in test_stop_emits_stop_handled replace the time.sleep polling with a threading.Event (or a bounded polling helper) where the bus handler passed to h.bus.on("mycroft.stop.handled", ...) calls event.set(), and then assert event.wait(timeout) returned True; apply the same pattern to the other tests noted (lines 212-250, 383-418) and prefer mocking time or directly mutating play_start_time over using real sleeps.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@test/unittests/test_audio_harness.py`:
- Around line 205-210: The assertion currently checks h.backend.played_tracks
after the AudioServiceHarness context has exited; move the assertion into the
with block so the test validates queue behavior before teardown. Specifically,
inside test_queue_appends_to_played_tracks keep using AudioServiceHarness() as
h, call h.play(...) and h.queue(...), then assert "http://example.com/song2.mp3"
in h.backend.played_tracks while still inside the with block (before __exit__
runs) to avoid relying on teardown to preserve playback state.
- Around line 169-188: Replace real-time sleeps with deterministic waits/mocks:
in test_stop_after_guard_clears_playing set or patch the AudioService instance's
play_start_time (access via AudioServiceHarness()._service or similar) to a past
timestamp so the 1‑second guard is bypassed instead of time.sleep(1.1), then
call h.stop() and assert stopped; in test_stop_emits_stop_handled replace the
time.sleep polling with a threading.Event (or a bounded polling helper) where
the bus handler passed to h.bus.on("mycroft.stop.handled", ...) calls
event.set(), and then assert event.wait(timeout) returned True; apply the same
pattern to the other tests noted (lines 212-250, 383-418) and prefer mocking
time or directly mutating play_start_time over using real sleeps.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b17c6264-b196-45c8-afb0-480823f97de8
📒 Files selected for processing (5)
.github/workflows/build_tests.yml.github/workflows/release_workflow.yml.github/workflows/unit_tests.ymlpyproject.tomltest/unittests/test_audio_harness.py
- Added LIGHT_TEST_PIPELINE as a lightweight fallback when Adapt/Padatious are missing. - Updated MiniCroft to auto-fallback to LIGHT_TEST_PIPELINE if stages are missing. - Refactored PlaybackServiceHarness for better robustness (proper patch cleanup, timeout handling). - Added skip guard to audio harness tests to prevent failures when ovos-audio is not installed. - Fixed documentation path references and added prerequisites. - Added missing LICENSE (Apache-2.0) file. - Updated CI workflows to include audio extra for unit tests.
JarbasAl
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
docs/audio-testing.md (1)
94-109:⚠️ Potential issue | 🟡 MinorMake this example runnable as pasted.
Lines 105-108 still use
h.buswithout constructing anAudioServiceHarness, so the snippet fails withNameErrorwhen copied verbatim.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/audio-testing.md` around lines 94 - 109, The snippet uses h.bus but never constructs an AudioServiceHarness; create an instance (e.g., AudioServiceHarness()) and assign it to h before using h.bus, then run the bus interaction and teardown through that harness (use the harness' bus for on/emit/remove and ensure the harness is started/stopped as needed) so the functions _on_reply, done.wait and h.bus.remove operate on a valid harness instance.MAINTENANCE_REPORT.md (1)
12-20:⚠️ Potential issue | 🟡 MinorFix the repo paths in this entry.
These bullets still point to paths that do not exist in this repo, e.g.
ovoscope/ovoscope/__init__.py,ovoscope/pyproject.toml,ovoscope/docs/audio-testing.md, andovoscope/FAQ.md. The current tree usesovoscope/__init__.py,pyproject.toml,docs/audio-testing.md,FAQ.md, andtest/unittests/....🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MAINTENANCE_REPORT.md` around lines 12 - 20, Update the bullet list entries in MAINTENANCE_REPORT.md to use the repository's actual paths: replace occurrences of ovoscope/ovoscope/__init__.py with ovoscope/__init__.py, ovoscope/pyproject.toml with pyproject.toml, ovoscope/docs/audio-testing.md with docs/audio-testing.md, ovoscope/FAQ.md with FAQ.md, and adjust any test paths like ovoscope/test/unittests/... to test/unittests/... so each bullet points to the real file locations listed in this repo.AUDIT.md (1)
20-39:⚠️ Potential issue | 🟡 MinorRefresh these audit findings before landing the report.
Line 20 still says the repo is missing
LICENSE, and Lines 31-39 / 67-69 still list public-method annotations and LICENSE addition as open work, but this PR already addsLICENSEand annotates APIs likeget_minicroft(),End2EndTest.execute(),from_message(),from_path(), andsave(). As written, the report ships stale guidance.Also applies to: 67-69
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AUDIT.md` around lines 20 - 39, Update AUDIT.md to remove or mark as resolved the stale findings about the missing LICENSE and missing type annotations: delete or mark the "[MAJOR] Missing LICENSE file" entry as fixed and remove the "[MAJOR] No type annotations on any public method" lines referencing get_minicroft(), CaptureSession.capture(), CaptureSession.finish(), End2EndTest.execute(), End2EndTest.from_message(), End2EndTest.from_path(), and End2EndTest.save() (or add ✅ FIXED and brief evidence pointing to the new LICENSE file and the updated type-annotated signatures) so the audit reflects the current repo state.ovoscope/__init__.py (2)
914-932:⚠️ Potential issue | 🟠 MajorDon’t hide internal import regressions behind optional exports.
These blanket
except ImportErrorblocks still swallow real failures insideovoscope.audioorovoscope.listener. If either module breaks because of a bad internal import, the symbol just disappears fromovoscopeinstead of surfacing the regression.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovoscope/__init__.py` around lines 914 - 932, The current broad try/except ImportError around the ovoscope.audio and ovoscope.listener exports hides real import errors and causes symbols like MockAudioBackend, AudioServiceHarness, MockTTS, PlaybackServiceHarness, AudioCaptureSession, MiniListener, get_mini_listener, and ListenerTest to silently disappear; remove the blanket excepts so import failures surface (or, if these modules truly depend on optional third‑party packages, replace the blanket except with a narrow ModuleNotFoundError check that only swallows the specific missing dependency package name and re-raises any other ImportError). Ensure the imports for the listed symbols are done directly (no broad try/except) or implement the targeted ModuleNotFoundError logic so internal regressions are not masked.
420-444:⚠️ Potential issue | 🟠 MajorStop the instance when startup fails or times out.
MiniCroft.__init__()now mutates live config before startup completes. Ifcroft.start()raises or Line 438 times out, this helper exits without callingcroft.stop(), so the thread, bus, and patched config (lang,secondary_langs,pipeline_config, isolated XDG state) can leak into later tests.Possible fix
assert isinstance(skill_ids, list) croft = MiniCroft(skill_ids, *args, **kwargs) - croft.start() - deadline = time() + max_wait - while croft.status.state != ProcessState.READY: - if time() > deadline: - raise TimeoutError( - f"MiniCroft did not reach READY in {max_wait}s — " - f"check skill startup logs (skill_ids={skill_ids})" - ) - sleep(0.1) - return croft + try: + croft.start() + deadline = time() + max_wait + while croft.status.state != ProcessState.READY: + if time() > deadline: + raise TimeoutError( + f"MiniCroft did not reach READY in {max_wait}s — " + f"check skill startup logs (skill_ids={skill_ids})" + ) + sleep(0.1) + return croft + except Exception: + croft.stop() + raise🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovoscope/__init__.py` around lines 420 - 444, The helper get_minicroft may leave a running/partially-initialized MiniCroft (and mutated global config) if croft.start() raises or the READY wait times out; update get_minicroft to ensure the instance is stopped on failure by wrapping startup and the readiness wait in a try/except/finally (or use a context manager) that calls croft.stop() when an exception is raised or when the timeout occurs; reference the croft variable, croft.start(), croft.stop(), and ProcessState.READY so the cleanup always runs before re-raising the error/TimeoutError.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ovoscope/__init__.py`:
- Around line 447-477: The file defines is_pipeline_available twice; remove the
duplicate (the second definition shown) so there is a single canonical
is_pipeline_available function and avoid the F811 overwrite/dual-source problem.
Locate the duplicate function named is_pipeline_available (the implementation
that builds installed_bases from
importlib.metadata.entry_points(group="opm.pipeline") and strips
"-high"/"-medium"/"-low" suffixes) and delete it, or consolidate any needed
logic into the original is_pipeline_available implementation so all call sites
reference one definition.
- Around line 148-160: The constructor currently conflates an explicit
default_pipeline=None with the absent-argument case; introduce a unique sentinel
(e.g., DEFAULT_PIPELINE_UNSET = object()) and change the signature to
default_pipeline: Optional[List[str]] = DEFAULT_PIPELINE_UNSET, then set
self._default_pipeline only when default_pipeline is DEFAULT_PIPELINE_UNSET by
falling back to is_pipeline_available(DEFAULT_TEST_PIPELINE) ?
DEFAULT_TEST_PIPELINE : LIGHT_TEST_PIPELINE; if default_pipeline is the sentinel
leave self._default_pipeline as None to preserve the explicit "no override"
behavior referenced by
TestMiniCroftPipelineIsolation.test_no_pipeline_override_when_none and the FAQ.
In `@ovoscope/audio.py`:
- Around line 573-595: The except block fails to clean up a partially
constructed PlaybackService (self.svc) which can leave its background thread
running; update the except handler to detect if self.svc was created and
properly tear it down before returning—for example, if hasattr(self, "svc") and
self.svc: call its shutdown/stop/close method (e.g., self.svc.shutdown() or
self.svc.stop() depending on the PlaybackService API), and if that API is
unavailable ensure you at least join the background thread
(self.svc.playback_thread.join(timeout)) to avoid leaks, then continue to stop
the _play_audio_patcher and close the bus as currently done.
---
Duplicate comments:
In `@AUDIT.md`:
- Around line 20-39: Update AUDIT.md to remove or mark as resolved the stale
findings about the missing LICENSE and missing type annotations: delete or mark
the "[MAJOR] Missing LICENSE file" entry as fixed and remove the "[MAJOR] No
type annotations on any public method" lines referencing get_minicroft(),
CaptureSession.capture(), CaptureSession.finish(), End2EndTest.execute(),
End2EndTest.from_message(), End2EndTest.from_path(), and End2EndTest.save() (or
add ✅ FIXED and brief evidence pointing to the new LICENSE file and the updated
type-annotated signatures) so the audit reflects the current repo state.
In `@docs/audio-testing.md`:
- Around line 94-109: The snippet uses h.bus but never constructs an
AudioServiceHarness; create an instance (e.g., AudioServiceHarness()) and assign
it to h before using h.bus, then run the bus interaction and teardown through
that harness (use the harness' bus for on/emit/remove and ensure the harness is
started/stopped as needed) so the functions _on_reply, done.wait and
h.bus.remove operate on a valid harness instance.
In `@MAINTENANCE_REPORT.md`:
- Around line 12-20: Update the bullet list entries in MAINTENANCE_REPORT.md to
use the repository's actual paths: replace occurrences of
ovoscope/ovoscope/__init__.py with ovoscope/__init__.py, ovoscope/pyproject.toml
with pyproject.toml, ovoscope/docs/audio-testing.md with docs/audio-testing.md,
ovoscope/FAQ.md with FAQ.md, and adjust any test paths like
ovoscope/test/unittests/... to test/unittests/... so each bullet points to the
real file locations listed in this repo.
In `@ovoscope/__init__.py`:
- Around line 914-932: The current broad try/except ImportError around the
ovoscope.audio and ovoscope.listener exports hides real import errors and causes
symbols like MockAudioBackend, AudioServiceHarness, MockTTS,
PlaybackServiceHarness, AudioCaptureSession, MiniListener, get_mini_listener,
and ListenerTest to silently disappear; remove the blanket excepts so import
failures surface (or, if these modules truly depend on optional third‑party
packages, replace the blanket except with a narrow ModuleNotFoundError check
that only swallows the specific missing dependency package name and re-raises
any other ImportError). Ensure the imports for the listed symbols are done
directly (no broad try/except) or implement the targeted ModuleNotFoundError
logic so internal regressions are not masked.
- Around line 420-444: The helper get_minicroft may leave a
running/partially-initialized MiniCroft (and mutated global config) if
croft.start() raises or the READY wait times out; update get_minicroft to ensure
the instance is stopped on failure by wrapping startup and the readiness wait in
a try/except/finally (or use a context manager) that calls croft.stop() when an
exception is raised or when the timeout occurs; reference the croft variable,
croft.start(), croft.stop(), and ProcessState.READY so the cleanup always runs
before re-raising the error/TimeoutError.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: df63120f-f2bd-4697-899c-3dac03844b5a
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (9)
AUDIT.mdFAQ.mdLICENSEMAINTENANCE_REPORT.mddocs/audio-testing.mdovoscope/__init__.pyovoscope/audio.pytest/unittests/test_audio_harness.pytest/unittests/test_minicroft.py
- MiniListener: persist stt_instance and use as default in listen() - MiniCroft: ensure config rollback on __init__ failure - MiniCroft: make stop() safe for incomplete initialization - Imports: improve ImportError handling for optional components - Tests: replace real-time sleeps with deterministic waits in test_audio_harness.py - Docs: update index.md to reflect STT support in MiniListener - Maintenance: fix duplicate prefixes in MAINTENANCE_REPORT.md
- MiniCroft: implement DEFAULT_PIPELINE_UNSET sentinel for default_pipeline - MiniCroft: refactor get_minicroft to ensure cleanup on startup failure - PlaybackServiceHarness: ensure cleanup on __enter__ failure - Imports: refine ModuleNotFoundError handling for optional dependencies - Tests: refactor multiple-speak test for better robustness - Docs: make wait_for_response example fully runnable in audio-testing.md - Audit: update AUDIT.md documenting fixed issues and next steps
This was referenced Mar 11, 2026
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
New Features
Documentation
Tests
Maintenance