feat: support all ovos core services + CLI tooling for ovoscope#46
Merged
Conversation
…ture Phase 1: - ovoscope/cli.py — `ovoscope` CLI (record, run, diff, validate, coverage) - ovoscope/diff.py — fixture differ (MessageDiff, FixtureDiffResult, diff_fixtures) - ovoscope/phal.py — PHAL harness (MiniPHAL context manager, PHALTest dataclass) Phase 2: - ovoscope/ocp.py — OCP harness (OCPTest, assert_ocp_query_response, HTTP mocking) - ovoscope/pipeline.py — pipeline harness (PipelineHarness, assert_matches/no_match) Phase 3: - ovoscope/coverage.py — ecosystem coverage scanner (scan_workspace, EcosystemCoverageReport) - ovoscope/remote_recorder.py — live OVOS fixture recording (RemoteRecorder) - ovoscope/__init__.py — GUICaptureSession (gui.* capture, assert_page_shown etc.) pyproject.toml: add [project.scripts] entry point for `ovoscope` CLI New docs: docs/cli.md, docs/phal.md, docs/ocp.md, docs/pipeline.md docs/usage-guide.md: Patterns 9–12 (multi-skill, PHAL, OCP, GUI) New tests: test_diff.py (7), test_phal.py (8), test_coverage.py (11), test_cli.py (14) All 202 tests pass, no regressions. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…nt groups - Add _find_setup_pys() to discover repos that only have setup.py (no pyproject.toml) - Add _parse_setup_py_entry_points() with regex-based extraction and directory-name fallback for dynamic f-string entry points (common in OVOS skill repos) - Add legacy entry point groups: ovos.plugin.skill, ovos.plugin.phal, ovos.plugin.phal.admin, ovos.plugin.tts, ovos.plugin.stt, ovos.plugin.audio - Refactor scan_workspace() into _collect_repo() helper + dual pyproject/setup.py scan - Result: Skills workspace now shows 58 repos (was 4), PHAL shows 15 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThis PR introduces a comprehensive testing framework expansion for ovoscope, adding a CLI interface, multiple test harnesses (PHAL, OCP, Pipeline), fixture diffing utilities, remote recording, and VAD/WakeWord support. Seven new public modules are added alongside extensive documentation and test coverage. Changes
Sequence Diagram(s)sequenceDiagram
participant User as CLI User
participant CLI as ovoscope.cli
participant MiniCroft as MiniCroft
participant Bus as FakeBus
participant Skills as Loaded Skills
User->>CLI: record --live <utterance>
CLI->>CLI: _record_live()
CLI->>RemoteRecorder: connect() to live bus
RemoteRecorder-->>CLI: connected
CLI->>RemoteRecorder: record(utterance)
RemoteRecorder->>Bus: subscribe to messages
RemoteRecorder->>Bus: emit recognizer_loop:utterance
Bus->>Skills: route message
Skills->>Bus: emit response messages
Bus->>RemoteRecorder: on_message() capture
RemoteRecorder-->>CLI: End2EndTest fixture
CLI->>CLI: write fixture file
CLI-->>User: exit(0)
sequenceDiagram
participant Test as Test Code
participant CLI as ovoscope.cli
participant MiniCroft as MiniCroft
participant Bus as FakeBus
participant Pipeline as Pipeline Stack
participant Skills as Skills
Test->>CLI: run --fixture fixture.json
CLI->>CLI: cmd_run()
CLI->>MiniCroft: initialize with fixture skills
MiniCroft->>Pipeline: load pipeline stages
Pipeline->>Bus: initialize
CLI->>Bus: emit recognizer_loop:utterance
Bus->>Pipeline: route utterance
Pipeline->>Skills: intent matching/activation
Skills->>Bus: emit skill handler messages
Bus->>CLI: capture messages
CLI->>CLI: validate expected_messages
CLI-->>Test: exit(0) or exit(1)
sequenceDiagram
participant Test as Test Code
participant PHALTest as PHALTest
participant MiniPHAL as MiniPHAL
participant FakeBus as FakeBus
participant Plugin as PHAL Plugin
Test->>PHALTest: execute()
PHALTest->>MiniPHAL: __enter__
MiniPHAL->>FakeBus: initialize
MiniPHAL->>Plugin: instantiate from plugin_ids
Plugin->>FakeBus: subscribe to events
MiniPHAL->>FakeBus: emit trigger_message
FakeBus->>Plugin: route message
Plugin->>FakeBus: emit response
FakeBus->>MiniPHAL: capture via _on_message()
MiniPHAL->>MiniPHAL: assert_emitted(expected_type)
MiniPHAL->>MiniPHAL: __exit__
PHALTest-->>Test: return captured messages
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
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)
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 |
The automated sentinels have completed their watch. 💂♂️I've aggregated the results of the automated checks for this PR below. 📋 Repo HealthA thorough inspection of the project's hygiene. 🧼 ✅ All required files present. Latest Version: ✅ 🔒 Security (pip-audit)Looking for any Trojan horses in the dependencies. 🐎 ✅ No known vulnerabilities found (68 packages scanned). ⚖️ License CheckEnsuring our project remains legally compliant. ✅ ✅ No license violations found (60 packages). License distribution: 14× MIT, 14× MIT License, 9× Apache Software License, 5× Apache-2.0, 4× BSD-3-Clause, 2× ISC License (ISCL), 2× PSF-2.0, 2× Python Software Foundation License, +7 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. 🏷️ Release PreviewThe release banner is being designed! 🎨 Current:
✅ PR title follows conventional commit format. 🚀 Release Channel Compatibility Predicted next version:
🔨 Build TestsI've poured the digital concrete for this build. 🏗️ ✅ All versions pass
Processing... Done! Have a productive day! ☕ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (6)
test/unittests/test_phal.py (1)
64-78: Use raw strings for regex patterns inpytest.raises(match=...).The patterns contain dots (
.) which are regex metacharacters matching any character. Use raw strings with escaped dots for exact matching.♻️ Proposed fix
def test_assert_emitted_raises_on_timeout(self): with MiniPHAL() as phal: - with pytest.raises(AssertionError, match="test.never"): + with pytest.raises(AssertionError, match=r"test\.never"): phal.assert_emitted("test.never", timeout=0.2) def test_assert_not_emitted_raises_when_present(self): with MiniPHAL() as phal: phal._bus.emit(Message("bad.message")) time.sleep(0.1) - with pytest.raises(AssertionError, match="bad.message"): + with pytest.raises(AssertionError, match=r"bad\.message"): phal.assert_not_emitted("bad.message", wait=0.05)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_phal.py` around lines 64 - 78, The test uses regex patterns with dots in pytest.raises(match=...) which treat '.' as "any char"; update the assertions in test_assert_emitted_raises_on_timeout and test_assert_not_emitted_raises_when_present to pass raw string regexes with escaped dots (e.g., r"test\.never" and r"bad\.message") to ensure exact literal matching when calling phal.assert_emitted(...) and phal.assert_not_emitted(...).ovoscope/__init__.py (1)
984-998: Consider narrowing the exception catch for deserialization failures.The bare
except Exceptionat line 993 is intentionally defensive, but could be narrowed to catch more specific exceptions (e.g.,json.JSONDecodeError,KeyError,TypeError) to avoid masking unexpected errors.♻️ Optional: Narrow exception handling
def _on_message(self, raw: Any) -> None: if isinstance(raw, str): try: msg = Message.deserialize(raw) - except Exception: + except (json.JSONDecodeError, KeyError, TypeError, ValueError): return else: msg = raw🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovoscope/__init__.py` around lines 984 - 998, In _on_message, avoid the bare except around Message.deserialize; replace it with a narrowed catch that only handles expected deserialization errors (e.g., json.JSONDecodeError, KeyError, TypeError, ValueError) so only malformed/invalid message payloads are swallowed and other unexpected exceptions are propagated; keep the behavior of returning on those known errors and continue appending to self.messages when any(msg.msg_type.startswith(p) for p in self.prefixes) succeeds.ovoscope/ocp.py (1)
113-118: Hardcoded sleep percentage may be unreliable.Using
time.sleep(self.timeout * 0.5)is arbitrary. Consider either:
- Using a message-based wait (listen for
ovos.common_play.startorcomplete.intent.failure)- Polling for expected messages with early exit
♻️ Consider event-driven wait instead of fixed sleep
import threading done = threading.Event() def _on_complete(msg): if msg.msg_type in ("ovos.common_play.start", "complete.intent.failure", "ovos.utterance.handled"): done.set() mc.bus.on("message", _on_complete) mc.bus.emit(src_msg) done.wait(timeout=self.timeout) mc.bus.remove("message", _on_complete)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovoscope/ocp.py` around lines 113 - 118, Replace the hardcoded time.sleep(self.timeout * 0.5) in the block that emits src_msg (inside the context using _apply_patches and before mc.stop()) with an event-driven or polling wait that exits early when expected OCP responses arrive: register a temporary message listener on mc.bus that sets a threading.Event (or similar) when a terminal message type is seen (e.g., "ovos.common_play.start", "complete.intent.failure", "ovos.utterance.handled"), emit src_msg, wait on that event with timeout=self.timeout, then remove the listener; alternatively implement a short-poll loop checking mc.bus for those messages and break early—ensure you reference and use the same mc, src_msg, _apply_patches, and self.timeout symbols so the wait replaces the hardcoded sleep and cleans up the listener before calling mc.stop().ovoscope/coverage.py (2)
405-407: Consider clearer deduplication pattern.The expression
not (i in seen or seen.add(i))exploits short-circuit evaluation and the fact thatset.add()returnsNone. While clever, this is a code smell that hinders readability.♻️ Proposed clearer alternative
# Deduplicate entry-point IDs - seen: set = set() - unique_ids = [i for i in ep_ids if not (i in seen or seen.add(i))] # type: ignore[func-returns-value] + seen: set = set() + unique_ids: List[str] = [] + for i in ep_ids: + if i not in seen: + seen.add(i) + unique_ids.append(i)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovoscope/coverage.py` around lines 405 - 407, The current deduplication expression in unique_ids uses a clever but obscure trick with seen.add() returning None; replace it with a clearer pattern: iterate ep_ids and append to unique_ids only when the id is not already in seen, adding it to seen as you go. Update the logic around the variables seen, ep_ids, and unique_ids (the list comprehension that builds unique_ids) so it uses an explicit for-loop or a simple helper to improve readability while preserving order and deduplication.
314-336: Consider adding debug logging for silent exception handlers.The broad
except Exception: passpatterns (lines 315-316, 334-335) provide resilience but make debugging difficult when parsing fails unexpectedly. Adding optional debug logging would help diagnose issues without breaking the scanner's robustness.💡 Example improvement
+import logging + +_LOG = logging.getLogger(__name__) + def _parse_entry_points(pyproject_path: str) -> Dict[str, List[str]]: ... except (ImportError, Exception): - pass + _LOG.debug("Failed to parse %s with tomllib, falling back to line scan", pyproject_path) ... except Exception: - pass + _LOG.debug("Fallback line-scan failed for %s", pyproject_path, exc_info=True) return eps🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovoscope/coverage.py` around lines 314 - 336, The two broad silent except blocks in the entry-point parsing fallback (the try/except around the initial return and the try/except while scanning pyproject) should log the caught exceptions at debug level instead of silently passing; update the except handlers to catch Exception as e and call a logger.debug or processLogger.debug with a clear message and exception info (e.g., "Failed to parse entry-points via importlib.metadata" and "Fallback pyproject scan failed for {pyproject_path}") while preserving the current behavior (still return eps). Ensure you reference the same symbols in the block (eps, current_group, pyproject_path) and either use an existing module logger or add one via logging.getLogger(__name__).test/unittests/test_cli.py (1)
51-99: Add handler-level tests forrecordandrun.This file covers parser wiring plus
diff/validate/coverage, but the lifecycle-heavy public paths inovoscope/cli.pyare still untested. A few mocked tests for_record_inprocess,_record_live, andcmd_runwould catch cleanup regressions and flag-default bugs much earlier.Also applies to: 146-231
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_cli.py` around lines 51 - 99, Add unit tests that exercise the lifecycle handlers _record_inprocess, _record_live, and cmd_run in addition to the parser tests: mock dependencies (file I/O, subprocesses, network, and any cleanup functions) and assert they are called/cleaned up and default flags are respected; for _record_inprocess and _record_live, call the handler via the parsed args from _build_parser for both live and non-live record invocations and verify output creation and teardown behavior (e.g., temporary files closed/deleted, streams stopped), and for cmd_run, invoke it with a parsed fixture arg and mock the run path to verify it returns/exits correctly and respects verbose/default flags. Ensure tests patch the same names (_record_inprocess, _record_live, cmd_run, _build_parser) imported by the CLI so the mocks are effective and include assertions for cleanup calls and default flag values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@MAINTENANCE_REPORT.md`:
- Around line 305-307: Remove the duplicate horizontal rule '---' that appears
immediately before the "## 2026-03-11 — Phase 1–3 Feature Additions" heading;
keep a single separator line, delete the second '---' so there is only one
markdown rule above that section heading.
In `@ovoscope/cli.py`:
- Around line 92-108: The code must always stop/disconnect MiniCroft and
RemoteRecorder in a finally block so subprocesses/sockets are not leaked: wrap
creation of mc via get_minicroft and any RemoteRecorder usage in try/finally
(check None before stopping) and move mc.stop() / recorder.disconnect() into the
finally so they run on all exceptions; apply the same pattern around
End2EndTest.from_message, End2EndTest.record/execute, and test.save calls
(symbols to update: get_minicroft, mc, End2EndTest.from_message,
End2EndTest.record/execute, mc.stop(), RemoteRecorder and its disconnect/stop
methods) and replicate this fix for the other occurrences noted (the blocks
around cmd_run and the ranges mentioned).
- Around line 170-177: The code rebuilds skill lists from message context
(variables skill_ids and all_skill_ids) instead of using the End2EndTest's
authoritative list; replace that logic to use test.skill_ids as the replay
source of truth (e.g., initialize skill_ids and any "all_skill_ids" usage from
test.skill_ids) and remove or stop relying on per-message context lookups
(msg.context.get("skill_id") / msg.data.get("skill_id")) so fixtures that depend
on stored test.skill_ids are loaded consistently.
- Around line 327-329: The CLI flag definition for "--ignore-context" uses
action="store_true" with default=True so args.ignore_context is always True;
change the argument to use argparse.BooleanOptionalAction (or define both
--ignore-context and --no-ignore-context) in the p_diff.add_argument call for
"--ignore-context" so the flag can be toggled from the CLI and
args.ignore_context reflects the user's choice; update the
p_diff.add_argument("--ignore-context", ...) invocation accordingly and remove
the conflicting default.
In `@ovoscope/diff.py`:
- Around line 141-156: The docstring for _load_messages incorrectly claims it
raises KeyError when "expected_messages" is absent, but the implementation uses
payload.get("expected_messages", []) so it returns an empty list instead; update
the _load_messages docstring (the Raises section) to remove KeyError and
document that missing "expected_messages" results in returning an empty list
(mention the use of payload.get("expected_messages", []) to justify the
behavior).
- Around line 159-165: The diff_fixtures function currently accepts strict_order
but always aligns fixtures by index; either remove the unused strict_order
parameter and its docs or implement the non-strict path: inside diff_fixtures,
branch on strict_order and when strict_order is False perform "first-match"
alignment by scanning the expected fixtures for the first unmatched item that
equals the actual (mark expected items as used) instead of using index-based
pairing, update any helper logic that builds pairs and the returned
FixtureDiffResult accordingly (refer to diff_fixtures, FixtureDiffResult and the
pairing/align-by-index code paths) and adjust tests/docs to reflect the chosen
behavior.
In `@ovoscope/ocp.py`:
- Around line 145-166: _build_mock_response currently ignores the mock_responses
map; change it so the inner json_side_effect accepts the request URL (or args
passed by requests.get/Session.request) and searches mock_responses for a key
that is a substring of that URL, returning the associated value (or {}
fallback), then set mock.json.side_effect to that function; also update the
caller/_build_patches to use side_effect on the mocked response (not
return_value) so each request triggers json_side_effect and returns per-URL
bodies.
In `@ovoscope/phal.py`:
- Around line 129-137: Replace the incorrect import of OVOSPHALPlugin and
instantiation with the proper PHALPlugin from
ovos_plugin_manager.templates.phal, i.e. import PHALPlugin and construct plugin
= PHALPlugin(bus=self._bus, config=cfg, plugin_id=plugin_id) (keep cfg from
self.config.get(plugin_id, {})); also update the warnings.warn call to include
stacklevel=2 so it reads warnings.warn(f"Failed to load PHAL plugin
{plugin_id!r}: {exc}", stacklevel=2) to point the warning at the caller.
In `@ovoscope/pipeline.py`:
- Around line 37-69: The _SinkSkill class is defined but never instantiated or
injected into MiniCroft, so its intent-capture handlers (_handle and
_handle_failure) are dead; either remove the class or instantiate and inject it
when creating the MiniCroft instance in PipelineHarness.__enter__ (or subscribe
it to the bus after startup) by constructing _SinkSkill(self._mc.bus) or passing
it via extra_skills to get_minicroft (so __ovoscope_sink__ is registered); also
ensure PipelineHarness.match() uses the _SinkSkill._last_match if you choose to
integrate it.
- Around line 104-113: The PipelineHarness.__enter__ call to get_minicroft uses
the wrong kwarg name and omits the stored config: change the call to pass the
pipeline under the name expected by MiniCroft (use
default_pipeline=self.pipeline or None) and also forward the stored
self.pipeline_config (e.g. default_pipeline_config=self.pipeline_config or None)
so per-stage overrides are applied; update the get_minicroft(...) invocation
accordingly.
In `@ovoscope/remote_recorder.py`:
- Around line 159-165: The current logic in record() treats a timeout as
successful if any message was captured (uses self._captured), which returns
partial data; change it to consider the wait result only: after calling
completed = self._done_event.wait(timeout=timeout) and removing the listener
(self._client.remove("message", self._on_message)), if completed is False raise
a TimeoutError regardless of self._captured so timed-out captures are not
silently treated as success; update the TimeoutError message to include the
utterance and timeout as before and ensure this check is done before returning
any captured fixture.
- Around line 167-171: End2EndTest is being instantiated without the required
skill_ids parameter; update the construction in remote_recorder.py to include a
skill_ids argument (e.g. skill_ids=<list of skill IDs derived from the recorder
or captured messages>) such as extracting unique skill ids from self._captured
(or using an existing recorder attribute like self._skill_ids if present) and
pass that list into End2EndTest(source_message=src,
expected_messages=list(self._captured), skill_ids=skill_ids) so the required
field is provided.
In `@test/unittests/test_diff.py`:
- Around line 37-43: The helper _write_fixture leaves the tempfile open (tmp)
before returning its path, which can block reopening on Windows; after json.dump
and tmp.flush() close the file handle by calling tmp.close() (or refactor to use
a with-context for tempfile.NamedTemporaryFile(..., delete=False)) so the
function returns a path to a closed file ready to be opened elsewhere.
---
Nitpick comments:
In `@ovoscope/__init__.py`:
- Around line 984-998: In _on_message, avoid the bare except around
Message.deserialize; replace it with a narrowed catch that only handles expected
deserialization errors (e.g., json.JSONDecodeError, KeyError, TypeError,
ValueError) so only malformed/invalid message payloads are swallowed and other
unexpected exceptions are propagated; keep the behavior of returning on those
known errors and continue appending to self.messages when
any(msg.msg_type.startswith(p) for p in self.prefixes) succeeds.
In `@ovoscope/coverage.py`:
- Around line 405-407: The current deduplication expression in unique_ids uses a
clever but obscure trick with seen.add() returning None; replace it with a
clearer pattern: iterate ep_ids and append to unique_ids only when the id is not
already in seen, adding it to seen as you go. Update the logic around the
variables seen, ep_ids, and unique_ids (the list comprehension that builds
unique_ids) so it uses an explicit for-loop or a simple helper to improve
readability while preserving order and deduplication.
- Around line 314-336: The two broad silent except blocks in the entry-point
parsing fallback (the try/except around the initial return and the try/except
while scanning pyproject) should log the caught exceptions at debug level
instead of silently passing; update the except handlers to catch Exception as e
and call a logger.debug or processLogger.debug with a clear message and
exception info (e.g., "Failed to parse entry-points via importlib.metadata" and
"Fallback pyproject scan failed for {pyproject_path}") while preserving the
current behavior (still return eps). Ensure you reference the same symbols in
the block (eps, current_group, pyproject_path) and either use an existing module
logger or add one via logging.getLogger(__name__).
In `@ovoscope/ocp.py`:
- Around line 113-118: Replace the hardcoded time.sleep(self.timeout * 0.5) in
the block that emits src_msg (inside the context using _apply_patches and before
mc.stop()) with an event-driven or polling wait that exits early when expected
OCP responses arrive: register a temporary message listener on mc.bus that sets
a threading.Event (or similar) when a terminal message type is seen (e.g.,
"ovos.common_play.start", "complete.intent.failure", "ovos.utterance.handled"),
emit src_msg, wait on that event with timeout=self.timeout, then remove the
listener; alternatively implement a short-poll loop checking mc.bus for those
messages and break early—ensure you reference and use the same mc, src_msg,
_apply_patches, and self.timeout symbols so the wait replaces the hardcoded
sleep and cleans up the listener before calling mc.stop().
In `@test/unittests/test_cli.py`:
- Around line 51-99: Add unit tests that exercise the lifecycle handlers
_record_inprocess, _record_live, and cmd_run in addition to the parser tests:
mock dependencies (file I/O, subprocesses, network, and any cleanup functions)
and assert they are called/cleaned up and default flags are respected; for
_record_inprocess and _record_live, call the handler via the parsed args from
_build_parser for both live and non-live record invocations and verify output
creation and teardown behavior (e.g., temporary files closed/deleted, streams
stopped), and for cmd_run, invoke it with a parsed fixture arg and mock the run
path to verify it returns/exits correctly and respects verbose/default flags.
Ensure tests patch the same names (_record_inprocess, _record_live, cmd_run,
_build_parser) imported by the CLI so the mocks are effective and include
assertions for cleanup calls and default flag values.
In `@test/unittests/test_phal.py`:
- Around line 64-78: The test uses regex patterns with dots in
pytest.raises(match=...) which treat '.' as "any char"; update the assertions in
test_assert_emitted_raises_on_timeout and
test_assert_not_emitted_raises_when_present to pass raw string regexes with
escaped dots (e.g., r"test\.never" and r"bad\.message") to ensure exact literal
matching when calling phal.assert_emitted(...) and phal.assert_not_emitted(...).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ea5dc912-41ba-40a8-9354-4567f430a149
📒 Files selected for processing (20)
FAQ.mdMAINTENANCE_REPORT.mddocs/cli.mddocs/ocp.mddocs/phal.mddocs/pipeline.mddocs/usage-guide.mdovoscope/__init__.pyovoscope/cli.pyovoscope/coverage.pyovoscope/diff.pyovoscope/ocp.pyovoscope/phal.pyovoscope/pipeline.pyovoscope/remote_recorder.pypyproject.tomltest/unittests/test_cli.pytest/unittests/test_coverage.pytest/unittests/test_diff.pytest/unittests/test_phal.py
Comment on lines
+104
to
+113
| def __enter__(self) -> "PipelineHarness": | ||
| """Start MiniCroft with the specified pipeline and no skills.""" | ||
| from ovoscope import get_minicroft | ||
| self._mc = get_minicroft( | ||
| skill_ids=[], | ||
| lang=self.lang, | ||
| pipeline=self.pipeline or None, | ||
| max_wait=60, | ||
| ) | ||
| return self |
There was a problem hiding this comment.
Critical: Wrong keyword argument name breaks pipeline configuration.
The get_minicroft() function forwards kwargs to MiniCroft.__init__, which expects default_pipeline (not pipeline). This causes the pipeline configuration to be silently ignored.
Additionally, self.pipeline_config (stored at line 96) is never passed to get_minicroft(), so per-stage configuration overrides documented in the constructor will not work.
🐛 Proposed fix
def __enter__(self) -> "PipelineHarness":
"""Start MiniCroft with the specified pipeline and no skills."""
from ovoscope import get_minicroft
self._mc = get_minicroft(
skill_ids=[],
lang=self.lang,
- pipeline=self.pipeline or None,
+ default_pipeline=self.pipeline or None,
+ pipeline_config=self.pipeline_config or None,
max_wait=60,
)
return self🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ovoscope/pipeline.py` around lines 104 - 113, The PipelineHarness.__enter__
call to get_minicroft uses the wrong kwarg name and omits the stored config:
change the call to pass the pipeline under the name expected by MiniCroft (use
default_pipeline=self.pipeline or None) and also forward the stored
self.pipeline_config (e.g. default_pipeline_config=self.pipeline_config or None)
so per-stage overrides are applied; update the get_minicroft(...) invocation
accordingly.
…ues) Critical fixes: - ocp.py:166 - Implement unused mock_responses parameter to fix HTTP mocking - phal.py:137 - Fix incorrect import (OVOSPHALPlugin -> PHALPlugin) + add stacklevel - pipeline.py:69 - Instantiate _SinkSkill and pass via extra_skills to MiniCroft - pipeline.py:113 - Use correct kwarg name (pipeline -> default_pipeline) - remote_recorder.py:171 - Add required skill_ids argument to End2EndTest constructor Major fixes: - cli.py:71-108 - Add try/finally to ensure MiniCroft cleanup in _record_inprocess - cli.py:111-144 - Add try/finally to ensure RemoteRecorder cleanup in _record_live - cli.py:147-197 - Add try/finally to ensure MiniCroft cleanup in cmd_run - cli.py:170 - Use test.skill_ids instead of rebuilding from message context - cli.py:328 - Fix --ignore-context flag logic (change to --include-context with correct semantics) - remote_recorder.py:159-165 - Fail on any timeout, not just when no messages captured Minor fixes: - diff.py:150-152 - Fix docstring (remove non-existent KeyError from Raises) - diff.py:159-164 - Remove unused strict_order parameter - test_diff.py:43 - Close NamedTemporaryFile before returning path - MAINTENANCE_REPORT.md:304-305 - Remove duplicate markdown separator All 45 unit tests pass. No functional regressions. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
JarbasAl
changed the title
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
ovoscope/pipeline.py (1)
37-69:_SinkSkillappears unused after the fix is applied.The
_SinkSkillclass and its_last_matchattribute are never actually read byPipelineHarness.match(). Thematch()method (lines 135-181) uses its own local_capturehandler andcapturedlist instead of_SinkSkill._last_match.Consider removing
_SinkSkillentirely since the currentmatch()implementation is self-contained.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovoscope/pipeline.py` around lines 37 - 69, Remove the now-unused internal fallback class _SinkSkill (including its __init__, _handle, _handle_failure, and _last_match attribute) because PipelineHarness.match uses its own _capture handler and local captured list; delete any remaining instantiation or references to "__ovoscope_sink__" or _SinkSkill so there are no orphaned registrations on the bus, and run tests to ensure no other code relies on _last_match.ovoscope/remote_recorder.py (1)
66-73: Consider documenting or aligning_EOF_TYPESwithDEFAULT_EOF.
RemoteRecorder._EOF_TYPEScontains 6 message types, whileovoscope/__init__.py:DEFAULT_EOFcontains only["ovos.utterance.handled"]. This means live recordings will terminate on a broader set of conditions than in-process fixtures expect.This may be intentional (live recording needs more termination signals), but could cause fixtures recorded via
--liveto behave differently when replayed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovoscope/remote_recorder.py` around lines 66 - 73, RemoteRecorder._EOF_TYPES currently includes six message types while DEFAULT_EOF in ovos.__init__ only lists "ovos.utterance.handled", causing different termination behavior between live recordings and fixtures; update RemoteRecorder (class RemoteRecorder, attribute _EOF_TYPES) to either (a) align with DEFAULT_EOF by importing and using DEFAULT_EOF, (b) make _EOF_TYPES configurable via an initializer arg that defaults to DEFAULT_EOF, or (c) add a clear docstring/comment on RemoteRecorder._EOF_TYPES explaining why the broader set is required and how it differs from DEFAULT_EOF so maintainers know the intent and can opt-in to matching behavior when needed.
🤖 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/cli.py`:
- Around line 92-95: The call to get_minicroft uses the wrong keyword: replace
the pipeline keyword with default_pipeline so the selected pipeline is actually
passed (i.e., call get_minicroft(skill_ids, lang=lang,
default_pipeline=pipeline, max_wait=60)); update the call site in function/area
where get_minicroft is invoked to use default_pipeline and keep existing
TimeoutError handling unchanged.
In `@ovoscope/ocp.py`:
- Around line 145-171: The mock currently reads mock.url which is never set, so
_build_mock_response's json_side_effect should derive the requested URL from the
call args (or set mock.url when the mock is invoked) and not rely on a
pre-existing attribute; update json_side_effect in _build_mock_response to
inspect its positional args (e.g. use args[0] if present) to match against
mock_responses, and change the test patching in _build_patches to install the
mock via side_effect (a callable that receives the request URL, assigns it to
mock.url if you want continuity, and returns the mock) instead of using
return_value=mock so URL info is captured correctly by the matcher.
In `@ovoscope/phal.py`:
- Around line 129-140: The current code incorrectly instantiates the PHAL base
class with plugin_id= and thus creates a no-op plugin; instead use the
ovos_plugin_manager.phal discovery/loader to load the real implementation by
entry-point ID and pass the correct constructor arg name (name=) plus config and
bus. Replace the direct PHALPlugin(...) creation in the block handling plugin_id
with a call to the phal plugin loader (from ovos_plugin_manager.phal import
load_phal_plugin or equivalent discovery function), retrieve the concrete plugin
class for the given plugin_id, instantiate it with name=plugin_id,
config=self.config.get(plugin_id, {}), and bus=self._bus, and keep the existing
exception/warning handling if loading fails.
In `@ovoscope/pipeline.py`:
- Around line 109-122: The _SinkSkill is being instantiated with bus=None which
causes _SinkSkill.__init__ to call bus.on(...) and raise AttributeError; also
self.pipeline_config is not forwarded to get_minicroft(). Fix by changing
_SinkSkill so its __init__ does not call bus.on when bus is None (defer
registration), add a method like register_bus(self, bus) that performs bus.on
subscriptions when the bus becomes available, and in the pipeline creation code
call sink_skill.register_bus(self._mc.bus) after self._mc is created (or
alternatively instantiate _SinkSkill only after get_minicroft returns). Also
pass self.pipeline_config into get_minicroft(...) (replace
default_pipeline=self.pipeline or None with
default_pipeline=self.pipeline_config) so the pipeline configuration is actually
used; reference symbols: _SinkSkill, __init__, register_bus, sink_skill,
get_minicroft, self._mc, self.pipeline_config, and match().
---
Nitpick comments:
In `@ovoscope/pipeline.py`:
- Around line 37-69: Remove the now-unused internal fallback class _SinkSkill
(including its __init__, _handle, _handle_failure, and _last_match attribute)
because PipelineHarness.match uses its own _capture handler and local captured
list; delete any remaining instantiation or references to "__ovoscope_sink__" or
_SinkSkill so there are no orphaned registrations on the bus, and run tests to
ensure no other code relies on _last_match.
In `@ovoscope/remote_recorder.py`:
- Around line 66-73: RemoteRecorder._EOF_TYPES currently includes six message
types while DEFAULT_EOF in ovos.__init__ only lists "ovos.utterance.handled",
causing different termination behavior between live recordings and fixtures;
update RemoteRecorder (class RemoteRecorder, attribute _EOF_TYPES) to either (a)
align with DEFAULT_EOF by importing and using DEFAULT_EOF, (b) make _EOF_TYPES
configurable via an initializer arg that defaults to DEFAULT_EOF, or (c) add a
clear docstring/comment on RemoteRecorder._EOF_TYPES explaining why the broader
set is required and how it differs from DEFAULT_EOF so maintainers know the
intent and can opt-in to matching behavior when needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d2b5ff60-253a-4ce9-8e46-69f34070779e
📒 Files selected for processing (8)
MAINTENANCE_REPORT.mdovoscope/cli.pyovoscope/diff.pyovoscope/ocp.pyovoscope/phal.pyovoscope/pipeline.pyovoscope/remote_recorder.pytest/unittests/test_diff.py
🚧 Files skipped from review as they are similar to previous changes (1)
- test/unittests/test_diff.py
Comment on lines
+92
to
+95
| try: | ||
| mc = get_minicroft(skill_ids, lang=lang, pipeline=pipeline, max_wait=60) | ||
| except TimeoutError: | ||
| _die("MiniCroft did not reach READY state in time.") |
There was a problem hiding this comment.
Wrong keyword argument: pipeline should be default_pipeline.
get_minicroft() expects default_pipeline parameter (per ovoscope/__init__.py:456-484), not pipeline. This causes the pipeline configuration to be silently ignored.
🐛 Proposed fix
print(f"[record] Loading skills: {skill_ids}")
try:
- mc = get_minicroft(skill_ids, lang=lang, pipeline=pipeline, max_wait=60)
+ mc = get_minicroft(skill_ids, lang=lang, default_pipeline=pipeline, max_wait=60)
except TimeoutError:
_die("MiniCroft did not reach READY state in time.")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| try: | |
| mc = get_minicroft(skill_ids, lang=lang, pipeline=pipeline, max_wait=60) | |
| except TimeoutError: | |
| _die("MiniCroft did not reach READY state in time.") | |
| try: | |
| mc = get_minicroft(skill_ids, lang=lang, default_pipeline=pipeline, max_wait=60) | |
| except TimeoutError: | |
| _die("MiniCroft did not reach READY state in time.") |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ovoscope/cli.py` around lines 92 - 95, The call to get_minicroft uses the
wrong keyword: replace the pipeline keyword with default_pipeline so the
selected pipeline is actually passed (i.e., call get_minicroft(skill_ids,
lang=lang, default_pipeline=pipeline, max_wait=60)); update the call site in
function/area where get_minicroft is invoked to use default_pipeline and keep
existing TimeoutError handling unchanged.
Comment on lines
+145
to
+171
| def _build_mock_response(mock_responses: Dict[str, Any]) -> MagicMock: | ||
| """Create a mock ``requests.Response`` that returns configured JSON bodies. | ||
|
|
||
| When the mock is called with a URL, it checks if any key from | ||
| *mock_responses* appears as a substring of the URL and returns the | ||
| corresponding value. Falls back to an empty dict. | ||
|
|
||
| Args: | ||
| mock_responses: URL-substring → response body mapping. | ||
|
|
||
| Returns: | ||
| A :class:`unittest.mock.MagicMock` mimicking ``requests.Response``. | ||
| """ | ||
| mock = MagicMock() | ||
|
|
||
| def json_side_effect(*_args: Any, **_kwargs: Any) -> Any: | ||
| # Match URL against mock_responses keys (as substrings) | ||
| url = str(mock.url) if hasattr(mock, 'url') else "" | ||
| for key, value in mock_responses.items(): | ||
| if key in url: | ||
| return value | ||
| return {} | ||
|
|
||
| mock.json.side_effect = json_side_effect | ||
| mock.status_code = 200 | ||
| mock.ok = True | ||
| return mock |
There was a problem hiding this comment.
HTTP mock URL matching is broken — mock.url is never set.
The json_side_effect at line 162 reads mock.url to match against mock_responses keys, but mock.url is never assigned. When patch(target, return_value=mock) is used, the mock is returned directly without capturing the request URL.
As a result, str(mock.url) returns "<MagicMock ...>" which won't match any URL patterns, causing all responses to return {}.
🐛 Proposed fix: Use side_effect to capture URL
-def _build_mock_response(mock_responses: Dict[str, Any]) -> MagicMock:
+def _build_mock_response(mock_responses: Dict[str, Any]) -> Any:
"""Create a mock ``requests.Response`` that returns configured JSON bodies."""
- mock = MagicMock()
-
- def json_side_effect(*_args: Any, **_kwargs: Any) -> Any:
- # Match URL against mock_responses keys (as substrings)
- url = str(mock.url) if hasattr(mock, 'url') else ""
- for key, value in mock_responses.items():
- if key in url:
- return value
- return {}
-
- mock.json.side_effect = json_side_effect
- mock.status_code = 200
- mock.ok = True
- return mock
+ def mock_get(url: str, *args: Any, **kwargs: Any) -> MagicMock:
+ response = MagicMock()
+ response.status_code = 200
+ response.ok = True
+ # Match URL against mock_responses keys
+ body = {}
+ for pattern, data in mock_responses.items():
+ if pattern in url:
+ body = data
+ break
+ response.json.return_value = body
+ return response
+ return mock_getAlso update _build_patches to use side_effect:
for target in targets:
- patches.append(patch(target, return_value=mock_response))
+ patches.append(patch(target, side_effect=mock_response))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ovoscope/ocp.py` around lines 145 - 171, The mock currently reads mock.url
which is never set, so _build_mock_response's json_side_effect should derive the
requested URL from the call args (or set mock.url when the mock is invoked) and
not rely on a pre-existing attribute; update json_side_effect in
_build_mock_response to inspect its positional args (e.g. use args[0] if
present) to match against mock_responses, and change the test patching in
_build_patches to install the mock via side_effect (a callable that receives the
request URL, assigns it to mock.url if you want continuity, and returns the
mock) instead of using return_value=mock so URL info is captured correctly by
the matcher.
Comment on lines
+129
to
+140
| try: | ||
| from ovos_plugin_manager.templates.phal import PHALPlugin # type: ignore | ||
| cfg = self.config.get(plugin_id, {}) | ||
| plugin = PHALPlugin(bus=self._bus, config=cfg, plugin_id=plugin_id) | ||
| return plugin | ||
| except Exception as exc: | ||
| import warnings | ||
| warnings.warn( | ||
| f"Failed to load PHAL plugin {plugin_id!r}: {exc}", | ||
| stacklevel=2, | ||
| ) | ||
| return None |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
ovos-plugin-manager find_phal_plugins function signature
💡 Result:
ovos-plugin-manager defines find_phal_plugins with no parameters:
def find_phal_plugins() -> dict:
...It returns a dict mapping plugin names to entry points. [1]
Sources: [1] (github.com)
Citations:
🏁 Script executed:
curl -s https://raw.githubusercontent.com/OpenVoiceOS/ovos-plugin-manager/dev/ovos_plugin_manager/templates/phal.py | head -50Repository: TigreGotico/ovoscope
Length of output: 1788
Parameter mismatch and wrong approach for loading PHAL plugins.
Two issues with the plugin instantiation:
-
PHALPluginconstructor usesname=parameter, notplugin_id=. This will raiseTypeError: unexpected keyword argument 'plugin_id'. -
PHALPluginis a base class template — instantiating it directly creates an empty plugin that does nothing. To load actual plugins by entry-point ID, use the discovery functions fromovos_plugin_manager.phal.
🔧 Proposed fix using plugin discovery
def _instantiate_plugin(self, plugin_id: str) -> Optional[Any]:
try:
- from ovos_plugin_manager.templates.phal import PHALPlugin # type: ignore
+ from ovos_plugin_manager.phal import find_phal_plugins # type: ignore
cfg = self.config.get(plugin_id, {})
- plugin = PHALPlugin(bus=self._bus, config=cfg, plugin_id=plugin_id)
+ plugins = find_phal_plugins()
+ if plugin_id not in plugins:
+ raise ValueError(f"PHAL plugin {plugin_id!r} not found")
+ plugin_class = plugins[plugin_id]
+ plugin = plugin_class(bus=self._bus, config=cfg, name=plugin_id)
return plugin
except Exception as exc:🧰 Tools
🪛 Ruff (0.15.5)
[warning] 133-133: Consider moving this statement to an else block
(TRY300)
[warning] 134-134: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ovoscope/phal.py` around lines 129 - 140, The current code incorrectly
instantiates the PHAL base class with plugin_id= and thus creates a no-op
plugin; instead use the ovos_plugin_manager.phal discovery/loader to load the
real implementation by entry-point ID and pass the correct constructor arg name
(name=) plus config and bus. Replace the direct PHALPlugin(...) creation in the
block handling plugin_id with a call to the phal plugin loader (from
ovos_plugin_manager.phal import load_phal_plugin or equivalent discovery
function), retrieve the concrete plugin class for the given plugin_id,
instantiate it with name=plugin_id, config=self.config.get(plugin_id, {}), and
bus=self._bus, and keep the existing exception/warning handling if loading
fails.
Comment on lines
+109
to
+122
| sink_skill = _SinkSkill(bus=None) # bus set after MiniCroft creation | ||
|
|
||
| self._mc = get_minicroft( | ||
| skill_ids=[], | ||
| lang=self.lang, | ||
| default_pipeline=self.pipeline or None, | ||
| extra_skills={"__ovoscope_sink__": sink_skill}, | ||
| max_wait=60, | ||
| ) | ||
|
|
||
| # Update sink skill's bus reference now that MiniCroft is created | ||
| if self._mc is not None: | ||
| sink_skill.bus = self._mc.bus | ||
|
|
There was a problem hiding this comment.
Critical: _SinkSkill initialization with bus=None will raise AttributeError.
_SinkSkill.__init__ (lines 49-50) calls bus.on(...) to register handlers. When instantiated with bus=None at line 109, this will raise AttributeError: 'NoneType' object has no attribute 'on' before the bus reference can be updated at line 121.
Additionally, self.pipeline_config (stored at line 96) is never passed to get_minicroft().
🐛 Proposed fix
def __enter__(self) -> "PipelineHarness":
"""Start MiniCroft with the specified pipeline and no skills."""
from ovoscope import get_minicroft
- # Inject internal sink skill to capture matched intents
- sink_skill = _SinkSkill(bus=None) # bus set after MiniCroft creation
-
self._mc = get_minicroft(
skill_ids=[],
lang=self.lang,
default_pipeline=self.pipeline or None,
- extra_skills={"__ovoscope_sink__": sink_skill},
+ pipeline_config=self.pipeline_config or None,
max_wait=60,
)
- # Update sink skill's bus reference now that MiniCroft is created
- if self._mc is not None:
- sink_skill.bus = self._mc.bus
+ # Create and wire sink skill after MiniCroft has a bus
+ self._sink = _SinkSkill(bus=self._mc.bus)
return selfAlso update _SinkSkill to allow deferred handler registration, or remove it entirely since match() already uses direct bus subscriptions.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ovoscope/pipeline.py` around lines 109 - 122, The _SinkSkill is being
instantiated with bus=None which causes _SinkSkill.__init__ to call bus.on(...)
and raise AttributeError; also self.pipeline_config is not forwarded to
get_minicroft(). Fix by changing _SinkSkill so its __init__ does not call bus.on
when bus is None (defer registration), add a method like register_bus(self, bus)
that performs bus.on subscriptions when the bus becomes available, and in the
pipeline creation code call sink_skill.register_bus(self._mc.bus) after self._mc
is created (or alternatively instantiate _SinkSkill only after get_minicroft
returns). Also pass self.pipeline_config into get_minicroft(...) (replace
default_pipeline=self.pipeline or None with
default_pipeline=self.pipeline_config) so the pipeline configuration is actually
used; reference symbols: _SinkSkill, __init__, register_bus, sink_skill,
get_minicroft, self._mc, self.pipeline_config, and match().
JarbasAl
added a commit
that referenced
this pull request
Mar 11, 2026
Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
MAINTENANCE_REPORT.md (1)
322-357: Keep the report in chronological order.This new March 11 entry is appended after March 8/9/10 sections, which breaks the descending timeline the rest of the file uses. Moving it alongside the other
2026-03-11entries will make the changelog much easier to scan.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@MAINTENANCE_REPORT.md` around lines 322 - 357, The new "## 2026-03-11 — Phase 1–3 Feature Additions" section is appended out of chronological order; move this entire block (starting at the "## 2026-03-11 — Phase 1–3 Feature Additions" header and including the lists of New modules, Extended modules, New docs, New tests and pyproject change) so it sits with the other 2026-03-11 entries in the file to restore descending timeline order; ensure you preserve the exact header and content (including references like ovoscope/cli.py, ovoscope/diff.py, GUICaptureSession, and the test lists) and update no other text.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@MAINTENANCE_REPORT.md`:
- Around line 7-10: The total in the header is inconsistent with the category
breakdown: update the summary line to match the sum of the listed fixes
(Critical (5) + Major (6) + Minor (4) = 15) or adjust the category counts so
they sum to the stated total; verify the enumerated fixes (mock_responses in
ocp.py, wrong import in phal.py, instantiation of _SinkSkill, kwarg fix in
pipeline.py, add skill_ids in remote_recorder.py, three finally blocks in
cli.py, use of test.skill_ids, --ignore-context flag logic, timeout handling,
docstring fixes, removed unused params, closed file handles, removed duplicate
separator) are correctly counted after your change.
In `@test/unittests/test_audio_harness.py`:
- Around line 43-56: The current try/except around "from ovoscope.audio import
..." swallows any ImportError/ModuleNotFoundError and hides real regressions;
change the handler to only suppress errors caused by the optional ovos_audio
package being missing. In the except block for ImportError/ModuleNotFoundError
(around the import of AudioCaptureSession, AudioServiceHarness,
MockAudioBackend, MockTTS, PlaybackServiceHarness, SILENT_WAV) inspect the
exception (e.name or str(e)) and if it does not indicate the missing
"ovos_audio" module re-raise the exception; only when the missing module is
ovos_audio set AUDIO_AVAILABLE=False and stub the symbols, otherwise raise so
real import/export/API errors in ovoscope.audio fail tests loudly.
---
Nitpick comments:
In `@MAINTENANCE_REPORT.md`:
- Around line 322-357: The new "## 2026-03-11 — Phase 1–3 Feature Additions"
section is appended out of chronological order; move this entire block (starting
at the "## 2026-03-11 — Phase 1–3 Feature Additions" header and including the
lists of New modules, Extended modules, New docs, New tests and pyproject
change) so it sits with the other 2026-03-11 entries in the file to restore
descending timeline order; ensure you preserve the exact header and content
(including references like ovoscope/cli.py, ovoscope/diff.py, GUICaptureSession,
and the test lists) and update no other text.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 411a0d4f-d418-40cc-a0ea-ff356e794cd4
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
MAINTENANCE_REPORT.mdpyproject.tomltest/unittests/test_audio_harness.py
Comment on lines
+7
to
+10
| - Fixed all 13 CodeRabbit review comments from PR #46: | ||
| - **Critical (5)**: Fixed mock_responses in ocp.py, wrong import in phal.py, instantiate _SinkSkill, use correct kwarg in pipeline.py, add skill_ids to remote_recorder.py | ||
| - **Major (6)**: Added resource cleanup finally blocks in cli.py (3 functions), use test.skill_ids instead of rebuilding, fixed --ignore-context flag logic, improve timeout handling | ||
| - **Minor (4)**: Fixed docstring mismatches, removed unused parameters, closed file handles, removed duplicate markdown separator |
There was a problem hiding this comment.
Reconcile the review-comment totals.
Line 7 says 13 comments were fixed, but the category counts on Lines 8-10 add up to 15. Please update either the total or the breakdown so the report stays accurate.
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 8-8: Spaces inside emphasis markers
(MD037, no-space-in-emphasis)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@MAINTENANCE_REPORT.md` around lines 7 - 10, The total in the header is
inconsistent with the category breakdown: update the summary line to match the
sum of the listed fixes (Critical (5) + Major (6) + Minor (4) = 15) or adjust
the category counts so they sum to the stated total; verify the enumerated fixes
(mock_responses in ocp.py, wrong import in phal.py, instantiation of _SinkSkill,
kwarg fix in pipeline.py, add skill_ids in remote_recorder.py, three finally
blocks in cli.py, use of test.skill_ids, --ignore-context flag logic, timeout
handling, docstring fixes, removed unused params, closed file handles, removed
duplicate separator) are correctly counted after your change.
Comment on lines
+43
to
+56
| try: | ||
| from ovoscope.audio import ( | ||
| AudioCaptureSession, | ||
| AudioServiceHarness, | ||
| MockAudioBackend, | ||
| MockTTS, | ||
| PlaybackServiceHarness, | ||
| SILENT_WAV, | ||
| ) | ||
| except (ImportError, ModuleNotFoundError) as e: | ||
| # Gracefully handle ovos_audio import failures | ||
| AUDIO_AVAILABLE = False | ||
| AudioCaptureSession = AudioServiceHarness = MockAudioBackend = \ | ||
| MockTTS = PlaybackServiceHarness = SILENT_WAV = object |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== test/unittests/test_audio_harness.py (guard under review) ==="
sed -n '31,60p' test/unittests/test_audio_harness.py
echo
echo "=== ovoscope/__init__.py (current package-level optional-dependency guard) ==="
sed -n '921,936p' ovoscope/__init__.py
echo
echo "=== ovoscope/audio.py (symbols imported by the test) ==="
rg -n '^(class (AudioCaptureSession|AudioServiceHarness|MockAudioBackend|MockTTS|PlaybackServiceHarness)\b|SILENT_WAV\s*:)' ovoscope/audio.pyRepository: TigreGotico/ovoscope
Length of output: 2032
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "=== Imports from ovos_audio in ovoscope/audio.py ==="
rg 'from ovos_audio|import ovos_audio' ovoscope/audio.py | head -20
echo
echo "=== Exception handler current structure ==="
sed -n '43,56p' test/unittests/test_audio_harness.pyRepository: TigreGotico/ovoscope
Length of output: 778
🌐 Web query:
Python ModuleNotFoundError vs ImportError e.name attribute availability
💡 Result:
In Python 3.3+, ImportError gained the .name (and .path) attributes; they’re settable via keyword-only constructor args and otherwise may be left unset (commonly None). In Python 3.6+, ModuleNotFoundError was added as a subclass of ImportError, so it has the same .name attribute behavior. [1]
Practical implications:
- Python ≥ 3.6: both
ImportErrorandModuleNotFoundErrorobjects supporte.name(but it may beNone). - Python 3.3–3.5:
ImportErrorsupportse.name;ModuleNotFoundErrordoesn’t exist. - Python ≤ 3.2 / Python 2:
e.nameis not available.
Robust pattern across versions / custom-raised import errors:
try:
import missing_module
except ImportError as e:
mod = getattr(e, "name", None) # safe across versions
path = getattr(e, "path", None)Source: Python “Built-in Exceptions” docs for ImportError and ModuleNotFoundError. [1]
Don't turn every import regression into a skipped test.
Lines 43–56 swallow all ImportErrors from ovoscope.audio. That means a real breakage—such as a broken export, API drift in ovos-audio, or an internal import bug in ovoscope/audio.py—will silently mark audio as unavailable and skip the entire test suite instead of failing loudly. This should only suppress failures caused by the missing optional ovos_audio dependency itself.
The package-level handler in ovoscope/__init__.py (lines 921–936) already demonstrates the correct pattern: it re-raises any ImportError that is not caused by a missing ovos_audio module. The test should follow the same discipline.
Proposed fix
except (ImportError, ModuleNotFoundError) as e:
- # Gracefully handle ovos_audio import failures
+ # Only suppress failures caused by the optional ovos_audio dependency.
+ if isinstance(e, ModuleNotFoundError) and getattr(e, "name", None) and getattr(e.name, "startswith", lambda x: False)("ovos_audio"):
- AUDIO_AVAILABLE = False
- AudioCaptureSession = AudioServiceHarness = MockAudioBackend = \
- MockTTS = PlaybackServiceHarness = SILENT_WAV = object
+ AUDIO_AVAILABLE = False
+ AudioCaptureSession = AudioServiceHarness = MockAudioBackend = \
+ MockTTS = PlaybackServiceHarness = SILENT_WAV = object
+ else:
+ raise🧰 Tools
🪛 Ruff (0.15.5)
[error] 52-52: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
🤖 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 43 - 56, The current
try/except around "from ovoscope.audio import ..." swallows any
ImportError/ModuleNotFoundError and hides real regressions; change the handler
to only suppress errors caused by the optional ovos_audio package being missing.
In the except block for ImportError/ModuleNotFoundError (around the import of
AudioCaptureSession, AudioServiceHarness, MockAudioBackend, MockTTS,
PlaybackServiceHarness, SILENT_WAV) inspect the exception (e.name or str(e)) and
if it does not indicate the missing "ovos_audio" module re-raise the exception;
only when the missing module is ovos_audio set AUDIO_AVAILABLE=False and stub
the symbols, otherwise raise so real import/export/API errors in ovoscope.audio
fail tests loudly.
Now that ovos-audio has released a stable version that fixes the signal module dependency issue, simply require the stable release instead of adding workarounds or error handling. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
- Add MockVADEngine: silence = all-zero bytes, speech = any non-zero - Add MockHotWordEngine: fires after trigger_after updates, auto-resets - Extend MiniListener with vad_instance / ww_instances params - Add is_silence(), extract_speech(), detect_wakeword(), scan_for_wakeword() - Add VADTest and WakeWordTest declarative test dataclasses - Extend get_mini_listener() factory with vad/ww plugin and instance params - Make ovos_dinkum_listener import lazy so VAD/WW tests work standalone - Add 41 unit tests (test_listener_vad_ww.py) — all passing Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
- ocp.md: document execute() return type; clarify patch_targets format (dotted Python path of usage site, same as unittest.mock.patch); add aiohttp/httpx example - pipeline.md: document assert_matches(intent_type) as substring match; add source citations (ovoscope/pipeline.py:LINE) to all methods - cli.md: fix --ignore-context → --include-context flag name and explain when to use it; clarify validate pydantic fallback trigger condition - end2end-test.md, minicroft.md, capture-session.md: add ovoscope/__init__.py:LINE citations; document finish() idempotency - listener.md: add full VAD/WakeWord API section (MockVADEngine, MockHotWordEngine, VADTest, WakeWordTest) with examples and line citations; update constructor table with vad_instance/ww_instances params; fix stale line numbers; remove incorrect "no VAD/WW support" claim - index.md: add gui-testing.md link; expose GUICaptureSession and VAD/WW helpers in Public API section; fix "Does NOT Do" for VAD/WW - QUICK_FACTS.md: add entry-point groups table; update test count to 243 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
test/unittests/test_listener_vad_ww.py (1)
318-326: Minor: Replace ambiguous Unicode character.Line 320 uses
×(multiplication sign U+00D7) instead ofx(Latin letter). While this renders correctly, it can cause issues in some editors and grep operations.📝 Suggested fix
- # 4 × 512-byte frames as flat bytes + # 4 x 512-byte frames as flat bytes🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_listener_vad_ww.py` around lines 318 - 326, The comment "4 × 512-byte frames as flat bytes" contains a Unicode multiplication sign (×); replace it with the ASCII letter "x" so the comment reads "4 x 512-byte frames as flat bytes" to avoid editor/grep issues—update the comment near the call to listener.scan_for_wakeword (frame_size=512) in the test_listener_vad_ww test block.docs/ocp.md (1)
34-46: Minor markdown formatting suggestion.The table on lines 36-46 would benefit from blank lines before and after for markdown linting compliance (MD058), though this doesn't affect rendering.
📝 Suggested formatting fix
### Fields + | Field | Type | Default | Description | |-------|------|---------|-------------| | `skill_ids` | `List[str]` | **required** | OCP skill IDs to load. | ... | `patch_targets` | `List[str]` | `[]` | Additional `requests`-like module paths to patch (dotted Python path to the callable to replace). | + ### `execute()` — `ovoscope/ocp.py:90`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/ocp.md` around lines 34 - 46, Add a blank line immediately before the markdown table under the "### Fields" heading and another blank line immediately after the table (the block listing `skill_ids`, `utterance`, `mock_responses`, `expected_media`, `expected_stream_url`, `lang`, `timeout`, `patch_targets`) so the table is separated from surrounding content to satisfy MD058 linting.
🤖 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`:
- Line 14: Add a blank line before the markdown table row that contains
"[gui-testing.md](gui-testing.md) | `GUICaptureSession`" so the table is
separated from the preceding content; edit the docs/index.md content and insert
a single empty line immediately above that table row (and optionally another
blank line after the table block) to ensure proper markdown rendering.
In `@ovoscope/listener.py`:
- Around line 614-622: The shutdown() method currently skips VAD cleanup; update
ovsopce/listener.py's shutdown() to clean up self._vad: if self._vad is not
None, try to call a shutdown() method on it (if present), otherwise call reset()
(e.g., MockVADEngine.reset()) as a fallback, catch and log/ignore exceptions,
and then clear self._vad (set to None) to ensure consistent cleanup alongside
transformers and wake-word engines.
---
Nitpick comments:
In `@docs/ocp.md`:
- Around line 34-46: Add a blank line immediately before the markdown table
under the "### Fields" heading and another blank line immediately after the
table (the block listing `skill_ids`, `utterance`, `mock_responses`,
`expected_media`, `expected_stream_url`, `lang`, `timeout`, `patch_targets`) so
the table is separated from surrounding content to satisfy MD058 linting.
In `@test/unittests/test_listener_vad_ww.py`:
- Around line 318-326: The comment "4 × 512-byte frames as flat bytes" contains
a Unicode multiplication sign (×); replace it with the ASCII letter "x" so the
comment reads "4 x 512-byte frames as flat bytes" to avoid editor/grep
issues—update the comment near the call to listener.scan_for_wakeword
(frame_size=512) in the test_listener_vad_ww test block.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c7b730cd-5c3c-4fa4-8b51-744f6221cf11
📒 Files selected for processing (16)
FAQ.mdMAINTENANCE_REPORT.mdQUICK_FACTS.mddocs/capture-session.mddocs/cli.mddocs/end2end-test.mddocs/gui-testing.mddocs/index.mddocs/listener.mddocs/minicroft.mddocs/ocp.mddocs/phal.mddocs/pipeline.mdovoscope/listener.pypyproject.tomltest/unittests/test_listener_vad_ww.py
✅ Files skipped from review due to trivial changes (3)
- docs/phal.md
- docs/listener.md
- docs/end2end-test.md
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/pipeline.md
- pyproject.toml
| | [audio-testing.md](audio-testing.md) | `AudioServiceHarness`, `PlaybackServiceHarness` — testing audio services | | ||
| | [listener.md](listener.md) | `MiniListener`, `get_mini_listener`, `ListenerTest` — testing audio transformer plugins and STT pipeline | | ||
| | [listener.md](listener.md) | `MiniListener`, `get_mini_listener`, `ListenerTest`, `MockVADEngine`, `MockHotWordEngine`, `VADTest`, `WakeWordTest` — testing audio transformer plugins, STT pipeline, VAD, and wake-word | | ||
| | [gui-testing.md](gui-testing.md) | `GUICaptureSession` — asserting GUI page navigation and namespace values | |
There was a problem hiding this comment.
Add blank line before the table row for better markdown rendering.
The table at line 14 should be surrounded by blank lines according to markdown best practices.
📝 Suggested fix
| [audio-testing.md](audio-testing.md) | `AudioServiceHarness`, `PlaybackServiceHarness` — testing audio services |
+
| [listener.md](listener.md) | `MiniListener`, `get_mini_listener`, `ListenerTest`, `MockVADEngine`, `MockHotWordEngine`, `VADTest`, `WakeWordTest` — testing audio transformer plugins, STT pipeline, VAD, and wake-word |
| [gui-testing.md](gui-testing.md) | `GUICaptureSession` — asserting GUI page navigation and namespace values |
+
## Conceptual Model📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| | [gui-testing.md](gui-testing.md) | `GUICaptureSession` — asserting GUI page navigation and namespace values | | |
| | [audio-testing.md](audio-testing.md) | `AudioServiceHarness`, `PlaybackServiceHarness` — testing audio services | | |
| | [listener.md](listener.md) | `MiniListener`, `get_mini_listener`, `ListenerTest`, `MockVADEngine`, `MockHotWordEngine`, `VADTest`, `WakeWordTest` — testing audio transformer plugins, STT pipeline, VAD, and wake-word | | |
| | [gui-testing.md](gui-testing.md) | `GUICaptureSession` — asserting GUI page navigation and namespace values | | |
| ## Conceptual Model |
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 14-14: Tables should be surrounded by blank lines
(MD058, blanks-around-tables)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/index.md` at line 14, Add a blank line before the markdown table row
that contains "[gui-testing.md](gui-testing.md) | `GUICaptureSession`" so the
table is separated from the preceding content; edit the docs/index.md content
and insert a single empty line immediately above that table row (and optionally
another blank line after the table block) to ensure proper markdown rendering.
Comment on lines
614
to
+622
| def shutdown(self) -> None: | ||
| """Shut down all loaded transformer plugins gracefully.""" | ||
| self.transformers.shutdown() | ||
| """Shut down all loaded transformer and wake-word plugins gracefully.""" | ||
| if self.transformers is not None: | ||
| self.transformers.shutdown() | ||
| for engine in self._ww.values(): | ||
| try: | ||
| engine.shutdown() | ||
| except Exception: | ||
| pass |
There was a problem hiding this comment.
VAD engine cleanup missing in shutdown().
The shutdown() method handles transformer and wake-word engine cleanup but does not clean up the VAD engine stored in self._vad. While MockVADEngine has a reset() method (not a shutdown), real VAD plugins may require cleanup.
Consider adding VAD cleanup for consistency:
🛡️ Suggested fix
def shutdown(self) -> None:
"""Shut down all loaded transformer and wake-word plugins gracefully."""
if self.transformers is not None:
self.transformers.shutdown()
for engine in self._ww.values():
try:
engine.shutdown()
except Exception:
pass
+ if self._vad is not None:
+ try:
+ if hasattr(self._vad, 'shutdown'):
+ self._vad.shutdown()
+ elif hasattr(self._vad, 'reset'):
+ self._vad.reset()
+ except Exception:
+ pass🧰 Tools
🪛 Ruff (0.15.5)
[error] 621-622: try-except-pass detected, consider logging the exception
(S110)
[warning] 621-621: Do not catch blind exception: Exception
(BLE001)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ovoscope/listener.py` around lines 614 - 622, The shutdown() method currently
skips VAD cleanup; update ovsopce/listener.py's shutdown() to clean up
self._vad: if self._vad is not None, try to call a shutdown() method on it (if
present), otherwise call reset() (e.g., MockVADEngine.reset()) as a fallback,
catch and log/ignore exceptions, and then clear self._vad (set to None) to
ensure consistent cleanup alongside transformers and wake-word engines.
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
Documentation
Tests