feat: Bus coverage + media testing#49
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a Bus Coverage feature (tracker, report models, pytest integration, CLI, docs, and tests), a media testing harness, GUI assertion helper, pipeline/diff enhancements, CI workflow updates, many documentation/maintenance artifacts, and extensive unit tests. No breaking removals; several public APIs extended. (50 words) Changes
Sequence DiagramsequenceDiagram
participant Test as End2EndTest
participant Tracker as BusCoverageTracker
participant Bus as FakeBus
participant Capture as CaptureSession
participant Report as BusCoverageReport
Note right of Test: pre-capture
Test->>Tracker: snapshot_listeners()
Test->>Tracker: start_tracking()
Tracker->>Bus: monkey-patch emit/register hooks
Capture->>Bus: emits & handler invocations during capture
Bus->>Tracker: intercepted registrations & emissions
Test->>Tracker: stop_tracking()
Tracker->>Bus: restore original emit/register
Test->>Tracker: record_session(responses, expected_messages)
Tracker->>Tracker: attribute events to skills
Tracker->>Report: build_report()
Report->>Test: assign bus_coverage_report / optional print
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)
📝 Coding Plan
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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Single command after `pip install ovoscope` installs the skill/agent definitions into Claude Code, Gemini CLI, and OpenCode. - ovoscope/setup_skill.py: install_claude(), install_gemini(), install_opencode(), uninstall_*(), detect_tools(), main() - ovoscope/skill_data/claude/: SKILL.md + scripts/ovoscope.sh + assets/docs/ - ovoscope/skill_data/gemini/: same format, project-level install - ovoscope/opencode/ovoscope.md: YAML frontmatter agent for OpenCode - pyproject.toml: ovoscope-setup entrypoint + package-data for skill_data/ - 26 unit tests (test_setup_skill.py) Usage: ovoscope-setup # auto-detect all tools on PATH ovoscope-setup --claude # Claude only (~/.claude/skills/ovoscope/) ovoscope-setup --gemini --path /my/workspace ovoscope-setup --opencode --path /my/project ovoscope-setup --list # show detected tools ovoscope-setup --uninstall --claude Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Remove all docs from skill_data/ — only SKILL.md + scripts/ are bundled - Remove OpenCode support - Add download_docs() using urllib.request (no new deps) to fetch docs from raw.githubusercontent.com at install time - Add --no-docs flag for offline/CI use - 32 unit tests, all network calls mocked Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…itHub - Remove bundled skill_data/ directory entirely; nothing is shipped in the wheel beyond setup_skill.py itself - SKILL.md is now downloaded from raw.githubusercontent.com at install time, keeping installed skills up-to-date with the live repo - Wrapper script (ovoscope.sh) generated inline — no local template needed - Shared _install_skill() helper used by both Claude and Gemini paths - Remove opencode support (out of scope) - Add --no-docs flag to skip docs fetch for offline / CI installs - Drop [tool.setuptools.package-data] from pyproject.toml (no bundled data) - Update unit tests: mock _fetch for SKILL.md download, remove TestSkillDataDir (no longer relevant), add test_skill_md_fetched_from_github Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Adds two-dimensional bus coverage measurement to ovoscope: - Listener coverage: which bus.on() registrations were invoked per test - Emitter coverage: which message types were observed vs asserted Key changes: - ovoscope/bus_coverage.py: BusCoverageTracker, BusCoverageReport, SkillBusCoverage, HandlerEntry, EmitterEntry; handles pyee v9 OrderedDict handler storage; attributes handlers via __self__ to minicroft.plugin_skills - ovoscope/__init__.py: track_bus_coverage / print_bus_coverage / bus_coverage_report fields on End2EndTest; hooks tracker in execute() - ovoscope/pytest_plugin.py: BusCoverageCollector, bus_coverage_session fixture, pytest_terminal_summary hook - ovoscope/cli.py: bus-coverage subcommand (table/json/verbose output) - docs/bus-coverage.md: full API reference with source citations - test/unittests/test_bus_coverage.py: 32 new tests (all passing) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
OVOS wraps all skill handlers in create_wrapper closures before registering on FakeBus, so handler.__self__ never points directly to the skill instance. Fix snapshot_listeners() to use the primary path: skill.events.events (EventContainer) — the authoritative list of (msg_type, handler) pairs maintained by ovos-workshop. Fallback path: bus introspection + closure scanning for __self__ (used when EventContainer is absent, e.g. injected test skills). Also: - _skill_instance_map() unwraps PluginSkillLoader.instance - Added _skill_id_from_closure() helper - Updated snapshot tests to use _FakeEventContainer stub Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Previously snapshot_listeners() only attributed handlers to loaded plugin skills. Now it covers every component that registers handlers on FakeBus — pipelines, core services, and the skill manager itself. Attribution strategy (three passes): 1. Skills with EventContainer: read skill.events.events directly (authoritative, handles create_wrapper closures) 2. Direct __self__ handlers: use type(owner).__name__ as component label; prefer owner.skill_id / owner.name attributes; look up in combined id→name map so skill instances without EventContainer still resolve to their skill_id 3. Closure scan: for handlers with no __self__, walk __closure__ cells for bound methods; uses the same combined map New component labels visible in reports: IntentService, AdaptPipeline, PadaciosoPipeline, PadatiousPipeline, ConverseService, FallbackService, StopService, MiniCroft (Thread-1) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Documents 4 critical, 7 major, 3 minor, 1 nitpick issues found in the bus coverage implementation: - async_responses excluded from emitter coverage - unattributed expected_messages silently dropped (no __core__ fallback) - registration-time handlers always show NOT TESTED (misleading) - non-skill messages excluded from emitter coverage (skill_id missing) - dead skill_map variable in record_session() - triple _get_bus_events() call in snapshot_listeners() - double-stop risk in cmd_bus_coverage() - pytest terminal hook uses private _arg2fixturedefs / cached_result - once() handlers invisible after firing before snapshot - ignore_messages silently excluded from emitter coverage - no JSON schema version field - docs gaps (async_responses, registration-time, pipeline, ignore_messages) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Critical fixes: - C1: pass capture.async_responses to record_session() so async messages are included in emitter coverage (__init__.py:666) - C2/C4: messages with no skill_id in context now attributed to __core__ bucket instead of silently dropped (bus_coverage.py:record_session) Major fixes: - M1: remove dead skill_map variable in record_session() - M2: cache _get_bus_events() once in snapshot_listeners() — was called 3x - M3: set test.managed=False before execute() in cli.py to prevent double-stop; removed redundant mc.stop() in except branch - M4: replace private pytest internals (_arg2fixturedefs, cached_result) with config._bus_coverage_reports list populated in fixture teardown - M7: add "schema_version": "1" to to_json() output Minor/nitpick fixes: - Dynamic column widths in print_report() using max skill_id length - Move print_bus_coverage to after all assertions in execute() - bus_coverage_report type hint changed to Optional["BusCoverageReport"] - Pass 2 now uses isinstance(owner, type) check instead of "type" string Documentation: - docs/bus-coverage.md Limitations section updated with all missing items: registration-time handlers, once() handlers, pipeline matching, ignore_messages exclusion, async_responses inclusion, __core__ bucket - AUDIT.md: all fixed items marked as resolved Tests: - 4 new tests: TestCoreAttributionBucket (3) + TestJsonSchemaVersion (1) - 37 tests total in test_bus_coverage.py, all pass Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Checking the status... all automated tasks are done! ✅I've aggregated the results of the automated checks for this PR below. 📋 Repo HealthI've checked the repo's eyesight (aka observability). 👓 ✅ All required files present. Latest Version: ✅ 🏷️ Release PreviewGet ready for the next big release! 🚀 Current:
✅ PR title follows conventional commit format. 🚀 Release Channel Compatibility Predicted next version:
🔒 Security (pip-audit)The security scan is now complete. 🏁 ✅ No known vulnerabilities found (68 packages scanned). ⚖️ License CheckEnsuring our project is well-protected legally. 🛡️ ✅ No license violations found (60 packages). License distribution: 14× MIT, 14× MIT License, 8× Apache Software License, 6× 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. 🔨 Build TestsThe build bots are giving this a thumbs up. 👍 ✅ All versions pass
🔍 LintThe automated gnomes have finished their shift. 🍄 ❌ ruff: issues found — see job log 📊 CoverageHow well do we know our own code? 🧠 ❌ 57.9% total coverage Files below 80% coverage (10 files)
Full report: download the Beep boop. See you in the next PR! 👋 |
…ntion Added (new): - build-tests.yml — build+test matrix Python 3.10–3.14, audio+pydantic extras - coverage.yml — PR coverage report (replaces unit_tests.yml) - coverage-pages.yml — push HTML coverage to gh-pages on dev push - lint.yml — ruff check on PRs - release-preview.yml — version bump preview on PR - repo-health.yml — required files/version block checks Removed (replaced or deprecated): - build_tests.yml → build-tests.yml (renamed + fixed test path) - unit_tests.yml → superseded by coverage.yml - coverage_pages.yml → coverage-pages.yml (updated to use coverage.yml@dev with deploy_pages: true; was using deprecated coverage-pages.yml@dev) - release_preview.yml → release-preview.yml - repo_health.yml → repo-health.yml - python-support.yml → deprecated; covered by build-tests.yml - skill-check.yml / ovoscope.yml → removed (ovoscope is a framework, not a skill) - conventional-label.yaml → conventional-label.yml (extension normalized) Renamed to hyphen convention (no content change): - downstream_check.yml → downstream-check.yml - license_check.yml → license-check.yml - pip_audit.yml → pip-audit.yml - publish_stable.yml → publish-stable.yml - release_workflow.yml → release-workflow.yml All workflows use OpenVoiceOS/gh-automations@dev refs. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ovoscope/__init__.py (1)
642-669:⚠️ Potential issue | 🟡 MinorReset
bus_coverage_reportbefore each execution.If the same
End2EndTestinstance is reused, a prior coverage report survives runs where tracking is disabled and can still be printed on Lines 795-796. Clear the field up front so it always reflects the current execution only.Suggested fix
def execute(self, timeout: int = 30) -> List[Message]: + self.bus_coverage_report = None if self.minicroft is None: self.minicroft = get_minicroft(self.skill_ids) self.managed = TrueAlso applies to: 795-796
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovoscope/__init__.py` around lines 642 - 669, Reset self.bus_coverage_report to None at the start of the run so prior reports don't survive when track_bus_coverage is false; specifically, add self.bus_coverage_report = None just before the block that checks self.track_bus_coverage (where _bus_tracker is initialized) and ensure any later printing paths (the code that references self.bus_coverage_report) see only the fresh value for the current execution. This change should be applied around the End2EndTest execution entry point that contains the _bus_tracker logic to ensure every run clears the previous report.
🧹 Nitpick comments (3)
ovoscope/bus_coverage.py (1)
665-665: Consider movingcollectionsimport to module level.The
import collectionsinside_iter_handlers()is executed on every call. Since this is a static method that may be called frequently during listener introspection, moving the import to the module level would be slightly more efficient.♻️ Suggested refactor
from __future__ import annotations +import collections import dataclasses import json from typing import Any, Dict, List, OptionalThen remove line 665.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovoscope/bus_coverage.py` at line 665, The import of collections inside the static method _iter_handlers causes repeated imports on every call; move the import statement (import collections) to the module level (top of ovoscope/bus_coverage.py) and remove the inline import from the _iter_handlers method so the function uses the module-level collections reference instead.ovoscope/cli.py (1)
366-416: Consider extracting shared report-merging logic.The merging logic here (lines 366-416) is nearly identical to
BusCoverageCollector.merged_report()inpytest_plugin.py(lines 133-188). Both merge byskill_id, sum invocation counts, and construct the same data structures.Consider extracting a shared
merge_reports(reports: List[BusCoverageReport]) -> BusCoverageReportfunction inbus_coverage.pyto eliminate duplication and ensure consistent behavior.♻️ Suggested approach
Add to
ovoscope/bus_coverage.py:def merge_reports(reports: List[BusCoverageReport]) -> BusCoverageReport: """Merge multiple BusCoverageReport instances into one. Sums invocation counts, observed counts, and asserted counts across all reports, grouped by skill_id. """ # ... extracted merging logic ...Then both
cli.pyandpytest_plugin.pycan call:from ovoscope.bus_coverage import merge_reports final_report = merge_reports(collected_reports)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovoscope/cli.py` around lines 366 - 416, The merging logic duplicated in ovoscope/cli.py (the block that builds merged_listeners/merged_observed/merged_asserted and constructs SkillBusCoverage/HandlerEntry/EmitterEntry into a BusCoverageReport) should be extracted into a single function merge_reports(reports: List[BusCoverageReport]) -> BusCoverageReport in ovoscope/bus_coverage.py; implement the same aggregation (sum invocation_count, observed_count, asserted_count grouped by skill_id and msg_type) and return a BusCoverageReport, then replace the inline code in cli.py and the BusCoverageCollector.merged_report() in pytest_plugin.py to call merge_reports(reports) to remove duplication and keep behavior consistent.ovoscope/__init__.py (1)
587-589: Use TYPE_CHECKING import for forward-reference best practice (optional improvement).Line 589 uses a string forward reference for
BusCoverageReport. While this is valid Python and will not trigger linting errors, following the TYPE_CHECKING pattern is a best practice for consistency with the rest of the codebase where external types are explicitly imported.Suggested improvement
+from typing import Union, List, Dict, Any, Optional, TYPE_CHECKING + +if TYPE_CHECKING: + from ovoscope.bus_coverage import BusCoverageReport🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovoscope/__init__.py` around lines 587 - 589, Replace the string forward-reference for BusCoverageReport on the bus_coverage_report field with an actual type import using the TYPE_CHECKING pattern: add "from typing import TYPE_CHECKING" and inside "if TYPE_CHECKING:" import BusCoverageReport, then change the annotation on bus_coverage_report to Optional[BusCoverageReport] (no quotes); keep the dataclasses.field(...) setup unchanged and ensure typing.Optional is already imported or imported if not.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@ovoscope/__init__.py`:
- Around line 642-669: Reset self.bus_coverage_report to None at the start of
the run so prior reports don't survive when track_bus_coverage is false;
specifically, add self.bus_coverage_report = None just before the block that
checks self.track_bus_coverage (where _bus_tracker is initialized) and ensure
any later printing paths (the code that references self.bus_coverage_report) see
only the fresh value for the current execution. This change should be applied
around the End2EndTest execution entry point that contains the _bus_tracker
logic to ensure every run clears the previous report.
---
Nitpick comments:
In `@ovoscope/__init__.py`:
- Around line 587-589: Replace the string forward-reference for
BusCoverageReport on the bus_coverage_report field with an actual type import
using the TYPE_CHECKING pattern: add "from typing import TYPE_CHECKING" and
inside "if TYPE_CHECKING:" import BusCoverageReport, then change the annotation
on bus_coverage_report to Optional[BusCoverageReport] (no quotes); keep the
dataclasses.field(...) setup unchanged and ensure typing.Optional is already
imported or imported if not.
In `@ovoscope/bus_coverage.py`:
- Line 665: The import of collections inside the static method _iter_handlers
causes repeated imports on every call; move the import statement (import
collections) to the module level (top of ovoscope/bus_coverage.py) and remove
the inline import from the _iter_handlers method so the function uses the
module-level collections reference instead.
In `@ovoscope/cli.py`:
- Around line 366-416: The merging logic duplicated in ovoscope/cli.py (the
block that builds merged_listeners/merged_observed/merged_asserted and
constructs SkillBusCoverage/HandlerEntry/EmitterEntry into a BusCoverageReport)
should be extracted into a single function merge_reports(reports:
List[BusCoverageReport]) -> BusCoverageReport in ovoscope/bus_coverage.py;
implement the same aggregation (sum invocation_count, observed_count,
asserted_count grouped by skill_id and msg_type) and return a BusCoverageReport,
then replace the inline code in cli.py and the
BusCoverageCollector.merged_report() in pytest_plugin.py to call
merge_reports(reports) to remove duplication and keep behavior consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fcc9a5d8-4675-443c-8fc7-12ecb94b9242
📒 Files selected for processing (10)
AUDIT.mdFAQ.mdMAINTENANCE_REPORT.mddocs/bus-coverage.mddocs/index.mdovoscope/__init__.pyovoscope/bus_coverage.pyovoscope/cli.pyovoscope/pytest_plugin.pytest/unittests/test_bus_coverage.py
… E2E tests
Adds ovoscope/media.py with:
- MockOCPBackend: no-op AudioBackend tracking state + simulate_end/invalid_stream helpers
- OCPPlayerHarness: context manager wrapping real OCPMediaPlayer on FakeBus with
all heavy deps mocked (AudioService, VideoService, WebService, MPRIS, GUI, Config)
- OCPCaptureSession: bus message capture for asserting OCP message sequences
- _TolerantPlaylist patch to handle Playlist("title") incompatibility in installed ovos_utils
Adds docs/media-testing.md mirroring audio-testing.md structure.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…rk distinction OCPPlayerHarness: - cork(): emits ovos.common_play.cork → handle_cork_request (pauses player) - uncork(): emits ovos.common_play.uncork → handle_uncork_request (resumes if _paused_on_duck) - Expanded docstrings for duck()/unduck() clarifying the PAUSED guard in handle_unduck_request (ovos_media/player.py:1228) and when restore_volume is and is not called docs/media-testing.md: - New "Duck / Unduck vs Cork / Uncork" section with side-by-side comparison, bus message tables, code examples, and the record_end auto-uncork pattern - Updated API reference table with cork/uncork entries Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
handle_unduck_request no longer requires state==PAUSED — restore_volume is now called whenever _paused_on_duck is True, covering the PLAYING (duck) path correctly. Remove the "design note" warning about the PAUSED guard and update the code example + API table to show the corrected behaviour. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ckaging - Fix pipeline.py race condition and timeout detection in match() - Add strict mode to diff.py _dict_diff() for extra key detection - Remove dead _skill_id_for_handler() from bus_coverage.py - Add test_media.py and test_remote_recorder.py (P2 coverage) - Fix deprecated Message import in test_phal.py - Create SUGGESTIONS.md (required by AGENTS.md) - Update QUICK_FACTS.md test count (243→348) - Expand docs/pipeline.md to full API reference - Fix docs/ocp.md OCPTest class name and add OCPPlayerHarness cross-ref - Update AUDIT.md with new findings - Add pyproject.toml URLs, package-data, pytest timeout - Make coverage.py fixture search recursive with Path.rglob - Add proper type annotations to pytest_plugin.py _reports - 348 unit tests pass (was 301) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
| uses: OpenVoiceOS/gh-automations/.github/workflows/build-tests.yml@dev | ||
| secrets: inherit | ||
| with: | ||
| python_versions: '["3.10", "3.11", "3.12", "3.13", "3.14"]' | ||
| install_extras: 'audio,pydantic' | ||
| test_path: 'test/unittests/' |
Check warning
Code scanning / CodeQL
Workflow does not contain permissions Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 months ago
In general, the fix is to add an explicit permissions: block to the workflow (either at the root or under the affected job) that grants only the minimal scopes required. For a typical build-and-test workflow that just checks out code and runs tests, contents: read is sufficient; if the reusable workflow needs more (e.g., to write checks or pull request comments), additional scopes could be added, but we cannot infer that from the snippet, so we start with a minimal, safe configuration.
The best, least intrusive fix here is to add a top-level permissions: block (aligned with on: and jobs:) in .github/workflows/build-tests.yml, setting contents: read. This applies to all jobs that don’t override permissions, including the build job using the reusable workflow, and does not change any other behavior of the workflow. Concretely, we should insert:
permissions:
contents: readafter the on: block (e.g., after line 6) or before jobs:. No new imports or additional definitions are required because this is just YAML configuration for GitHub Actions.
| @@ -5,6 +5,9 @@ | ||
| branches: [dev, master, main] | ||
| workflow_dispatch: | ||
|
|
||
| permissions: | ||
| contents: read | ||
|
|
||
| jobs: | ||
| build: | ||
| uses: OpenVoiceOS/gh-automations/.github/workflows/build-tests.yml@dev |
| uses: OpenVoiceOS/gh-automations/.github/workflows/coverage.yml@dev | ||
| secrets: inherit | ||
| with: | ||
| python_version: '3.11' | ||
| coverage_source: 'ovoscope' | ||
| test_path: 'test/unittests/' | ||
| install_extras: 'audio,pydantic' | ||
| min_coverage: 0 |
Check warning
Code scanning / CodeQL
Workflow does not contain permissions Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 months ago
In general, to fix this class of issue you add a permissions block either at the top level of the workflow (so it applies to all jobs without their own permissions) or within the specific job. The block should grant only the minimum access needed, typically contents: read for jobs that just check out and read code or run tests.
For this specific workflow, the best fix with minimal behavioral change is to define explicit read-only permissions at the workflow level, directly under name: or on:. Since this workflow delegates to a reusable workflow to run coverage, and there is no evidence that it needs any write access (no steps are shown that push commits, create releases, comment on PRs, etc.), we can safely restrict permissions to contents: read. If the reusable workflow needs additional scopes, they can be added later in that reusable workflow, but here we should at least limit the default token. Concretely, edit .github/workflows/coverage.yml to insert:
permissions:
contents: readnear the top (e.g., after name: Code Coverage), leaving the rest of the workflow untouched.
| @@ -1,5 +1,8 @@ | ||
| name: Code Coverage | ||
|
|
||
| permissions: | ||
| contents: read | ||
|
|
||
| on: | ||
| pull_request: | ||
| branches: [dev] |
| uses: OpenVoiceOS/gh-automations/.github/workflows/lint.yml@dev | ||
| secrets: inherit | ||
| with: | ||
| ruff: true | ||
| pre_commit: false # set true if .pre-commit-config.yaml exists |
Check warning
Code scanning / CodeQL
Workflow does not contain permissions Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 4 months ago
In general, the fix is to explicitly define a permissions block that grants the minimal GITHUB_TOKEN scopes required for this lint job, either at the workflow root (for all jobs) or under the specific job (lint). Since this workflow only delegates to a reusable lint workflow, the host workflow itself likely only needs read access to the repository contents.
The least intrusive and clearest fix is to add a permissions block at the job level for lint, directly under the lint: job configuration, alongside uses, secrets, and with. For a linting workflow that merely reads code, contents: read is usually sufficient; if the reusable workflow needs additional scopes (e.g., to write PR comments), those can be added there as well, but given the information provided, we will use a conservative contents: read. Concretely, in .github/workflows/lint.yml, after line 9 (lint:) and before line 10 (uses: OpenVoiceOS/...), insert:
permissions:
contents: readThis ensures the lint job (and thus the GITHUB_TOKEN available to the reusable workflow) is limited to read-only access to repository contents, satisfying the CodeQL recommendation without changing the existing behavior of the linting logic.
| @@ -7,6 +7,8 @@ | ||
|
|
||
| jobs: | ||
| lint: | ||
| permissions: | ||
| contents: read | ||
| uses: OpenVoiceOS/gh-automations/.github/workflows/lint.yml@dev | ||
| secrets: inherit | ||
| with: |
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (3)
.github/workflows/coverage.yml (1)
17-17: Consider enforcing a non-zero minimum coverage.
min_coverage: 0removes the gating value of this workflow. Even a low baseline helps prevent regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/coverage.yml at line 17, The workflow currently sets min_coverage: 0 which disables gating; change the min_coverage value from 0 to a non-zero threshold (e.g., 70 or your chosen baseline) in the workflow config so the coverage job fails when coverage drops below the baseline; update the min_coverage field and, if needed, document or expose it as a workflow input/variable so teams can adjust the threshold over time.ovoscope/media.py (1)
385-494: Replace fixed sleeps with condition-based waits.Every control helper sleeps 50 ms after emitting. That makes the harness both slower and flaky: synchronous paths always pay the delay, while slower CI can still read stale player/backend state after 50 ms. Waiting for a concrete bus event or state predicate will make these helpers deterministic.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovoscope/media.py` around lines 385 - 494, The helper methods (play, pause, resume, stop, next_track, prev_track, duck, unduck, cork, uncork, simulate_track_end, simulate_invalid_stream) currently use fixed time.sleep(0.05) which makes tests flaky; replace those sleeps with deterministic waits: after emitting the Message, wait for a concrete bus event or state predicate that indicates the action completed (e.g. use self.bus.wait_for(...) for the appropriate OCP event like ovos.common_play.state or recognizer_loop events, or poll a PlayerState/backend property until it reflects the expected change), include a sensible timeout and clear fallback to avoid hanging, and apply this change consistently across all listed methods.test/unittests/test_media.py (1)
14-24: Add at least oneOCPPlayerHarnesssmoke test.This module is positioned as the unit-test home for the new media harnesses, but it never imports or exercises
OCPPlayerHarness. The patch-heavy__enter__/__exit__path is the riskiest part ofovoscope.media, so a simple context/open/close test would buy much more confidence than another backend-state assertion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_media.py` around lines 14 - 24, Add a simple smoke test that imports OCPPlayerHarness and exercises its context manager and lifecycle methods: create a MockOCPBackend (or MagicMock backend) and a FakeBus/Message stub, instantiate OCPPlayerHarness with those dependencies, then use a with OCPPlayerHarness(...) as harness: block and call harness.open() and harness.close() (or just ensure exit runs without error) to validate __enter__/__exit__ paths; name the test like test_ocpplayerharness_context_manager and place it in test_media.py alongside existing tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/build-tests.yml:
- Around line 3-15: Add an explicit top-level permissions block for the
GITHUB_TOKEN to avoid relying on default scopes; because this workflow runs on
pull_request and delegates to the reusable job referenced by uses:
OpenVoiceOS/gh-automations/.github/workflows/build-tests.yml@dev (job: build),
add a permissions: mapping that grants only the least-privilege scopes required
(for example, contents: read and any other specific scopes you need such as
pull-requests: write or id-token: write) instead of the defaults so the token
exposure is minimized.
- Line 10: The workflow currently references the reusable workflow using a
mutable ref "OpenVoiceOS/gh-automations/.github/workflows/build-tests.yml@dev";
update that to an immutable commit SHA (replace "@dev" with the specific commit
hash) so the workflow is pinned and reproducible. Locate the line that calls the
reusable workflow (the "uses:
OpenVoiceOS/gh-automations/.github/workflows/build-tests.yml@dev" entry) and
replace the ref with the full commit SHA from the target repository, commit the
change, and verify the workflow still executes as expected.
In @.github/workflows/coverage-pages.yml:
- Line 13: The workflow references a mutable branch tag in the reusable workflow
declaration ("uses:
OpenVoiceOS/gh-automations/.github/workflows/coverage.yml@dev"); replace the
branch ref with a specific commit SHA to pin the reusable workflow. Locate the
"uses: OpenVoiceOS/gh-automations/.github/workflows/coverage.yml@dev" line and
replace "@dev" with the full commit SHA (e.g., "@<full-commit-sha>") from the
OpenVoiceOS/gh-automations repo (obtain via git ls-remote or the repo's commit
history) so the workflow is deterministic and not tied to a moving branch.
In @.github/workflows/coverage.yml:
- Around line 3-17: Add an explicit top-level permissions mapping to this
workflow (near the top alongside the existing on: block) to enforce
least-privilege instead of using defaults; for example, add a permissions: block
granting only the minimum scopes your reusable workflow needs (e.g., contents:
read and any other specific scopes required by the reused workflow) so the
coverage job (the coverage reusable workflow invoked with uses:
OpenVoiceOS/gh-automations/.github/workflows/coverage.yml@dev) does not inherit
broad default permissions.
- Line 10: Replace the mutable branch reference in the reusable workflow
invocation "uses: OpenVoiceOS/gh-automations/.github/workflows/coverage.yml@dev"
with a specific commit SHA (e.g., @<full-40-char-commit-sha>) so the workflow is
pinned to an immutable commit; update the "uses" value to use that exact SHA and
commit the change.
In @.github/workflows/lint.yml:
- Around line 3-14: Add an explicit least-privilege permissions block to the
workflow so the "lint" job doesn't rely on broad default token scopes: update
the top-level workflow to include a permissions mapping (e.g., set contents:
read, checks: write or as minimal needed for the OpenVoiceOS/gh-automations
action) and ensure the "lint" job still passes secrets: inherit and with inputs
(ruff, pre_commit) to the referenced reusable workflow; reference the "lint" job
and the existing uses: OpenVoiceOS/gh-automations/.github/workflows/lint.yml@dev
when adding the permissions block.
- Line 10: The workflow is using a mutable ref ("uses:
OpenVoiceOS/gh-automations/.github/workflows/lint.yml@dev") which should be
pinned to an immutable commit SHA; update the `uses` entry in this workflow (and
the other workflows referenced: build-tests, coverage, downstream-check,
license-check, pip-audit, publish-stable, release-preview, release-workflow,
repo-health) to point to a specific commit SHA of OpenVoiceOS/gh-automations
(replace the `@dev` suffix with `@<full-commit-sha>`), ensuring each reusable
workflow reference uses the exact same or appropriate commit SHA for
reproducible CI.
In `@docs/pipeline.md`:
- Around line 66-69: PipelineHarness.match() claims to handle "late success" but
currently returns None as soon as _failed is set; update the method so a prior
_failed Event does not cause an immediate return — instead prefer a later
success by waiting on _succeeded for the remaining timeout even if _failed is
set, and only treat failure as terminal if timeout elapses without _succeeded
being set. Modify the match() implementation (the function named match in class
PipelineHarness) to compute remaining timeout and call
_succeeded.wait(remaining) rather than immediately returning when _failed is
True (or loop waiting on _succeeded with small sleeps), and only return None
after timeout/final failure; alternatively, if you prefer not to change code,
soften the docs to remove the stronger "does not mask a subsequent late success"
wording and explicitly state the current behavior that a failure currently
causes immediate return.
In `@ovoscope/bus_coverage.py`:
- Around line 489-516: The patched bus.emit installed by
BusCoverage.start_tracking can remain wrapped if End2EndTest.execute raises an
exception, so update End2EndTest.execute (the block that calls
BusCoverage.start_tracking / stop_tracking) to guarantee restoration in all
cases by invoking BusCoverage.stop_tracking inside a finally block (or use a
context-manager pattern around start_tracking/stop_tracking); ensure you
reference the BusCoverage instance used by End2EndTest.execute and restore its
_original_emit back onto self._bus.emit via stop_tracking even when
assertions/timeouts occur so _bus.emit and _original_emit are always reset.
- Around line 546-557: The fallback that guesses a skill by scanning
self._observed (inside the for msg in expected_messages loop) should be removed
so unscoped expected messages are not attributed to an arbitrary skill; change
the logic in the loop that uses _skill_id_for_message(msg) to skip the
scanning-for-first-emitter block and immediately set skill_id = "__core__" when
_skill_id_for_message returns None (affecting expected_messages handling and
asserted_emitter_pct attribution), leaving _observed and _skill_id_for_message
intact.
In `@ovoscope/pipeline.py`:
- Around line 180-193: The watcher thread started for _wait_either can run
forever if done.wait(timeout=timeout) times out; fix by adding a dedicated stop
Event (e.g., stop_event) that _wait_either checks in its loop in addition to
_matched and _failed, and when the caller times out set stop_event.set() and
join the watcher (or set the watcher as daemon and explicitly stop it) so the
thread can exit cleanly; update the watcher creation/cleanup around
watcher/_wait_either/done/timeout (and apply the same change to the similar
block at the later lines) so no orphan daemon threads accumulate.
In `@ovoscope/pytest_plugin.py`:
- Around line 165-191: The merge loop omits skills that only appear in
asserted_data; update the skill_id iteration in the bus report builder (the loop
building skills and using HandlerEntry, EmitterEntry, SkillBusCoverage,
BusCoverageReport) to union asserted_data as well — i.e., iterate over
sorted(set(listener_data) | set(observed_data) | set(asserted_data)) so
asserted-only skills are included in the final report.
In `@pyproject.toml`:
- Line 57: The pyproject setting "timeout = 60" relies on the pytest-timeout
plugin but that package is not listed alongside the existing dev dependencies
"pytest" and "pytest-cov"; add "pytest-timeout" to the project's dev
dependencies (the same section where "pytest" and "pytest-cov" are declared) so
the timeout option is honored by test runs, specifying an appropriate version
constraint.
- Around line 43-44: The Documentation URL in pyproject.toml currently points to
/tree/master/docs which will be broken for this dev-branch PR; update the
"Documentation" value to a branch-agnostic path such as using HEAD (e.g.,
/tree/HEAD/docs) or point it to the dev branch (/tree/dev/docs) or a
relative/docs-only URL so the docs link remains valid while this PR is
open—leave the "Issue Tracker" entry unchanged.
In `@test/unittests/test_remote_recorder.py`:
- Around line 32-58: The mock's emit_side_effect currently replays the entire
messages_to_emit list on every client.emit call, masking duplicate-emission
bugs; modify _make_mock_client so emit_side_effect consumes the scripted
messages only once (e.g., capture messages_to_emit into a local queue/iterator
and on the first client.emit deliver each message to handlers, then no-op on
subsequent emits), keeping references to handlers and using the same handler
invocation logic as before.
---
Nitpick comments:
In @.github/workflows/coverage.yml:
- Line 17: The workflow currently sets min_coverage: 0 which disables gating;
change the min_coverage value from 0 to a non-zero threshold (e.g., 70 or your
chosen baseline) in the workflow config so the coverage job fails when coverage
drops below the baseline; update the min_coverage field and, if needed, document
or expose it as a workflow input/variable so teams can adjust the threshold over
time.
In `@ovoscope/media.py`:
- Around line 385-494: The helper methods (play, pause, resume, stop,
next_track, prev_track, duck, unduck, cork, uncork, simulate_track_end,
simulate_invalid_stream) currently use fixed time.sleep(0.05) which makes tests
flaky; replace those sleeps with deterministic waits: after emitting the
Message, wait for a concrete bus event or state predicate that indicates the
action completed (e.g. use self.bus.wait_for(...) for the appropriate OCP event
like ovos.common_play.state or recognizer_loop events, or poll a
PlayerState/backend property until it reflects the expected change), include a
sensible timeout and clear fallback to avoid hanging, and apply this change
consistently across all listed methods.
In `@test/unittests/test_media.py`:
- Around line 14-24: Add a simple smoke test that imports OCPPlayerHarness and
exercises its context manager and lifecycle methods: create a MockOCPBackend (or
MagicMock backend) and a FakeBus/Message stub, instantiate OCPPlayerHarness with
those dependencies, then use a with OCPPlayerHarness(...) as harness: block and
call harness.open() and harness.close() (or just ensure exit runs without error)
to validate __enter__/__exit__ paths; name the test like
test_ocpplayerharness_context_manager and place it in test_media.py alongside
existing tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c9fe6c7a-625e-43b8-bbe0-1f91f724e77e
📒 Files selected for processing (31)
.github/workflows/build-tests.yml.github/workflows/build_tests.yml.github/workflows/conventional-label.yml.github/workflows/coverage-pages.yml.github/workflows/coverage.yml.github/workflows/coverage_pages.yml.github/workflows/downstream-check.yml.github/workflows/license-check.yml.github/workflows/lint.yml.github/workflows/pip-audit.yml.github/workflows/publish-stable.yml.github/workflows/release-preview.yml.github/workflows/release-workflow.yml.github/workflows/repo-health.ymlAUDIT.mdMAINTENANCE_REPORT.mdQUICK_FACTS.mdSUGGESTIONS.mddocs/media-testing.mddocs/ocp.mddocs/pipeline.mdovoscope/bus_coverage.pyovoscope/coverage.pyovoscope/diff.pyovoscope/media.pyovoscope/pipeline.pyovoscope/pytest_plugin.pypyproject.tomltest/unittests/test_media.pytest/unittests/test_phal.pytest/unittests/test_remote_recorder.py
💤 Files with no reviewable changes (2)
- .github/workflows/coverage_pages.yml
- .github/workflows/build_tests.yml
✅ Files skipped from review due to trivial changes (5)
- docs/ocp.md
- docs/media-testing.md
- .github/workflows/conventional-label.yml
- QUICK_FACTS.md
- .github/workflows/release-preview.yml
| on: | ||
| pull_request: | ||
| branches: [dev, master, main] | ||
| workflow_dispatch: | ||
|
|
||
| jobs: | ||
| build: | ||
| uses: OpenVoiceOS/gh-automations/.github/workflows/build-tests.yml@dev | ||
| secrets: inherit | ||
| with: | ||
| python_versions: '["3.10", "3.11", "3.12", "3.13", "3.14"]' | ||
| install_extras: 'audio,pydantic' | ||
| test_path: 'test/unittests/' |
There was a problem hiding this comment.
Add explicit GITHUB_TOKEN permissions.
This workflow currently relies on default token scopes. Please define least-privilege permissions explicitly to reduce token exposure.
Suggested fix
on:
pull_request:
branches: [dev, master, main]
workflow_dispatch:
+permissions:
+ contents: read
+
jobs:
build:🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 10-15: Workflow does not contain permissions
Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {{}}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/build-tests.yml around lines 3 - 15, Add an explicit
top-level permissions block for the GITHUB_TOKEN to avoid relying on default
scopes; because this workflow runs on pull_request and delegates to the reusable
job referenced by uses:
OpenVoiceOS/gh-automations/.github/workflows/build-tests.yml@dev (job: build),
add a permissions: mapping that grants only the least-privilege scopes required
(for example, contents: read and any other specific scopes you need such as
pull-requests: write or id-token: write) instead of the defaults so the token
exposure is minimized.
|
|
||
| jobs: | ||
| build: | ||
| uses: OpenVoiceOS/gh-automations/.github/workflows/build-tests.yml@dev |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n .github/workflows/build-tests.yml | head -20Repository: TigreGotico/ovoscope
Length of output: 524
Pin reusable workflow to an immutable ref.
Using @dev is mutable and can change behavior unexpectedly across runs. Pin to a commit SHA for reproducibility and supply-chain safety.
Suggested fix
- uses: OpenVoiceOS/gh-automations/.github/workflows/build-tests.yml@dev
+ uses: OpenVoiceOS/gh-automations/.github/workflows/build-tests.yml@<commit-sha>🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 10-15: Workflow does not contain permissions
Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {{}}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/build-tests.yml at line 10, The workflow currently
references the reusable workflow using a mutable ref
"OpenVoiceOS/gh-automations/.github/workflows/build-tests.yml@dev"; update that
to an immutable commit SHA (replace "@dev" with the specific commit hash) so the
workflow is pinned and reproducible. Locate the line that calls the reusable
workflow (the "uses:
OpenVoiceOS/gh-automations/.github/workflows/build-tests.yml@dev" entry) and
replace the ref with the full commit SHA from the target repository, commit the
change, and verify the workflow still executes as expected.
| jobs: | ||
| unit_tests: | ||
| coverage_pages: | ||
| uses: OpenVoiceOS/gh-automations/.github/workflows/coverage.yml@dev |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
GitHub Actions reusable workflow pinning commit SHA vs branch reference best practices
💡 Result:
Best practice: prefer immutable refs (commit SHA) over mutable refs (branches)
1) If the reusable workflow is in a different repo
- Security-first default: pin the called workflow to a full commit SHA (immutable) to reduce supply-chain risk from a moved tag/branch or repo compromise. This is the same guidance GitHub gives for actions generally, and org policies can enforce SHA pinning. [2], [3]
- Avoid
@main,@master, or any branch ref for “trusted” behavior—branches are mutable. [2]
Example:
jobs:
call:
uses: org/central-workflows/.github/workflows/ci.yml@<FULL_COMMIT_SHA>2) If the reusable workflow is in the same repo
- You can call it without
owner/repo@ref; GitHub resolves it from the same commit as the caller workflow, which is effectively immutable for that run and avoids ref-drift. [1]
Example:
jobs:
call:
uses: ./.github/workflows/ci.yml3) When a branch/tag ref can be acceptable (trade-off)
- If you need easier updates across many repos, use a versioned tag or release branch (e.g.,
v1/v1.2.3) only if you also put governance around it (protected tags/branches, required reviews, restricted push) and accept the residual risk that the ref can still move unless you enforce immutability. GitHub’s security guidance still prefers SHA pinning for strongest protection. [2]
4) Organization enforcement
- GitHub Actions policies can require full commit SHAs for dependencies (including reusable workflow references), failing workflows that aren’t pinned. [3]
Rule of thumb:
- External or cross-repo reuse: pin to full SHA. [2], [3]
- Same-repo reuse: omit
@refand rely on same-commit resolution. [1]
Sources: GitHub Docs on reusable workflows/ref resolution [1]; GitHub Well-Architected Actions security recommendation on immutable pinning [2]; GitHub Changelog on SHA-pinning policy enforcement [3].
Pin reusable workflow to a full commit SHA.
@dev is a mutable branch reference that can change unexpectedly, introducing supply-chain risk. GitHub's security guidance recommends pinning external reusable workflows to a specific commit SHA for deterministic CI behavior.
Suggested fix
- uses: OpenVoiceOS/gh-automations/.github/workflows/coverage.yml@dev
+ uses: OpenVoiceOS/gh-automations/.github/workflows/coverage.yml@<commit-sha>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/coverage-pages.yml at line 13, The workflow references a
mutable branch tag in the reusable workflow declaration ("uses:
OpenVoiceOS/gh-automations/.github/workflows/coverage.yml@dev"); replace the
branch ref with a specific commit SHA to pin the reusable workflow. Locate the
"uses: OpenVoiceOS/gh-automations/.github/workflows/coverage.yml@dev" line and
replace "@dev" with the full commit SHA (e.g., "@<full-commit-sha>") from the
OpenVoiceOS/gh-automations repo (obtain via git ls-remote or the repo's commit
history) so the workflow is deterministic and not tied to a moving branch.
| on: | ||
| pull_request: | ||
| branches: [dev] | ||
| workflow_dispatch: | ||
|
|
||
| jobs: | ||
| coverage: | ||
| uses: OpenVoiceOS/gh-automations/.github/workflows/coverage.yml@dev | ||
| secrets: inherit | ||
| with: | ||
| python_version: '3.11' | ||
| coverage_source: 'ovoscope' | ||
| test_path: 'test/unittests/' | ||
| install_extras: 'audio,pydantic' | ||
| min_coverage: 0 |
There was a problem hiding this comment.
Define explicit workflow permissions.
This workflow should declare least-privilege permissions instead of relying on defaults.
Suggested fix
on:
pull_request:
branches: [dev]
workflow_dispatch:
+permissions:
+ contents: read
+
jobs:
coverage:📝 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.
| on: | |
| pull_request: | |
| branches: [dev] | |
| workflow_dispatch: | |
| jobs: | |
| coverage: | |
| uses: OpenVoiceOS/gh-automations/.github/workflows/coverage.yml@dev | |
| secrets: inherit | |
| with: | |
| python_version: '3.11' | |
| coverage_source: 'ovoscope' | |
| test_path: 'test/unittests/' | |
| install_extras: 'audio,pydantic' | |
| min_coverage: 0 | |
| on: | |
| pull_request: | |
| branches: [dev] | |
| workflow_dispatch: | |
| permissions: | |
| contents: read | |
| jobs: | |
| coverage: | |
| uses: OpenVoiceOS/gh-automations/.github/workflows/coverage.yml@dev | |
| secrets: inherit | |
| with: | |
| python_version: '3.11' | |
| coverage_source: 'ovoscope' | |
| test_path: 'test/unittests/' | |
| install_extras: 'audio,pydantic' | |
| min_coverage: 0 |
🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 10-17: Workflow does not contain permissions
Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {{}}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/coverage.yml around lines 3 - 17, Add an explicit
top-level permissions mapping to this workflow (near the top alongside the
existing on: block) to enforce least-privilege instead of using defaults; for
example, add a permissions: block granting only the minimum scopes your reusable
workflow needs (e.g., contents: read and any other specific scopes required by
the reused workflow) so the coverage job (the coverage reusable workflow invoked
with uses: OpenVoiceOS/gh-automations/.github/workflows/coverage.yml@dev) does
not inherit broad default permissions.
|
|
||
| jobs: | ||
| coverage: | ||
| uses: OpenVoiceOS/gh-automations/.github/workflows/coverage.yml@dev |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check if the file exists and examine its content around line 10
if [ -f ".github/workflows/coverage.yml" ]; then
echo "=== File exists. Content around line 10: ==="
cat -n .github/workflows/coverage.yml | sed -n '1,20p'
else
echo "File not found"
fiRepository: TigreGotico/ovoscope
Length of output: 590
🏁 Script executed:
# Check if there are other workflow files using similar patterns
echo "=== Searching for other workflow references to gh-automations ==="
rg "gh-automations" .github/workflows/ -A 1 -B 1Repository: TigreGotico/ovoscope
Length of output: 2985
Pin reusable workflow to a commit SHA.
@dev is a mutable branch reference; pinning to a commit SHA ensures deterministic CI behavior and prevents unexpected upstream changes.
Suggested fix
- uses: OpenVoiceOS/gh-automations/.github/workflows/coverage.yml@dev
+ uses: OpenVoiceOS/gh-automations/.github/workflows/coverage.yml@<commit-sha>🧰 Tools
🪛 GitHub Check: CodeQL
[warning] 10-17: Workflow does not contain permissions
Actions job or workflow does not limit the permissions of the GITHUB_TOKEN. Consider setting an explicit permissions block, using the following as a minimal starting point: {{}}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/coverage.yml at line 10, Replace the mutable branch
reference in the reusable workflow invocation "uses:
OpenVoiceOS/gh-automations/.github/workflows/coverage.yml@dev" with a specific
commit SHA (e.g., @<full-40-char-commit-sha>) so the workflow is pinned to an
immutable commit; update the "uses" value to use that exact SHA and commit the
change.
| # Wait for either a match or a failure signal | ||
| import threading as _threading | ||
| done = _threading.Event() | ||
|
|
||
| def _wait_either() -> None: | ||
| while not _matched.is_set() and not _failed.is_set(): | ||
| _matched.wait(timeout=0.05) | ||
| if _matched.is_set() or _failed.is_set(): | ||
| break | ||
| done.set() | ||
|
|
||
| watcher = _threading.Thread(target=_wait_either, daemon=True) | ||
| watcher.start() | ||
| timed_out = not done.wait(timeout=timeout) |
There was a problem hiding this comment.
Timeout path leaks watcher threads.
If no event arrives, done.wait(timeout=timeout) times out, but the watcher loop keeps running indefinitely. Repeated calls can accumulate orphan daemon threads.
Suggested fix
captured: List[Message] = []
_matched = threading.Event()
_failed = threading.Event()
+ done = threading.Event()
@@
def _on_success(msg: Any) -> None:
if isinstance(msg, str):
try:
msg = Message.deserialize(msg)
except Exception:
return
captured.append(msg)
_matched.set()
+ done.set()
def _on_failure(msg: Any) -> None:
_failed.set()
+ done.set()
@@
- # Wait for either a match or a failure signal
- import threading as _threading
- done = _threading.Event()
-
- def _wait_either() -> None:
- while not _matched.is_set() and not _failed.is_set():
- _matched.wait(timeout=0.05)
- if _matched.is_set() or _failed.is_set():
- break
- done.set()
-
- watcher = _threading.Thread(target=_wait_either, daemon=True)
- watcher.start()
timed_out = not done.wait(timeout=timeout)Also applies to: 199-203
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ovoscope/pipeline.py` around lines 180 - 193, The watcher thread started for
_wait_either can run forever if done.wait(timeout=timeout) times out; fix by
adding a dedicated stop Event (e.g., stop_event) that _wait_either checks in its
loop in addition to _matched and _failed, and when the caller times out set
stop_event.set() and join the watcher (or set the watcher as daemon and
explicitly stop it) so the thread can exit cleanly; update the watcher
creation/cleanup around watcher/_wait_either/done/timeout (and apply the same
change to the similar block at the later lines) so no orphan daemon threads
accumulate.
| skills = [] | ||
| for skill_id in sorted(set(listener_data) | set(observed_data)): | ||
| listeners = [ | ||
| HandlerEntry( | ||
| msg_type=mt, | ||
| handler_count=hc, | ||
| invocation_count=ic, | ||
| covered=ic > 0, | ||
| ) | ||
| for mt, (hc, ic) in sorted(listener_data.get(skill_id, {}).items()) | ||
| ] | ||
| all_emitted = set(observed_data.get(skill_id, {}).keys()) | set( | ||
| asserted_data.get(skill_id, {}).keys() | ||
| ) | ||
| emitters = [ | ||
| EmitterEntry( | ||
| msg_type=mt, | ||
| observed_count=observed_data.get(skill_id, {}).get(mt, 0), | ||
| asserted_count=asserted_data.get(skill_id, {}).get(mt, 0), | ||
| observed=observed_data.get(skill_id, {}).get(mt, 0) > 0, | ||
| asserted=asserted_data.get(skill_id, {}).get(mt, 0) > 0, | ||
| ) | ||
| for mt in sorted(all_emitted) | ||
| ] | ||
| skills.append(SkillBusCoverage(skill_id=skill_id, listeners=listeners, emitters=emitters)) | ||
|
|
||
| return BusCoverageReport(skills=skills) |
There was a problem hiding this comment.
Keep asserted-only skills in the merged session report.
The merge loop only unions listener_data and observed_data, so a skill that appears solely in asserted_data disappears from the final terminal summary. That hides the "asserted but not observed" case instead of surfacing it.
💡 Minimal fix
- for skill_id in sorted(set(listener_data) | set(observed_data)):
+ for skill_id in sorted(
+ set(listener_data) | set(observed_data) | set(asserted_data)
+ ):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@ovoscope/pytest_plugin.py` around lines 165 - 191, The merge loop omits
skills that only appear in asserted_data; update the skill_id iteration in the
bus report builder (the loop building skills and using HandlerEntry,
EmitterEntry, SkillBusCoverage, BusCoverageReport) to union asserted_data as
well — i.e., iterate over sorted(set(listener_data) | set(observed_data) |
set(asserted_data)) so asserted-only skills are included in the final report.
| Documentation = "https://github.com/TigreGotico/ovoscope/tree/master/docs" | ||
| "Issue Tracker" = "https://github.com/TigreGotico/ovoscope/issues" |
There was a problem hiding this comment.
Documentation URL references master branch.
The Documentation URL points to /tree/master/docs, but this PR targets the dev branch. If the docs (including docs/bus-coverage.md) are new additions, this link will be broken until dev is merged into master.
Consider using a branch-agnostic URL or ensuring docs exist on master before this PR merges:
-Documentation = "https://github.com/TigreGotico/ovoscope/tree/master/docs"
+Documentation = "https://github.com/TigreGotico/ovoscope/blob/dev/docs"Alternatively, keep as-is if master is the primary release branch and broken links during development are acceptable.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pyproject.toml` around lines 43 - 44, The Documentation URL in pyproject.toml
currently points to /tree/master/docs which will be broken for this dev-branch
PR; update the "Documentation" value to a branch-agnostic path such as using
HEAD (e.g., /tree/HEAD/docs) or point it to the dev branch (/tree/dev/docs) or a
relative/docs-only URL so the docs link remains valid while this PR is
open—leave the "Issue Tracker" entry unchanged.
|
|
||
| [tool.pytest.ini_options] | ||
| testpaths = ["test"] | ||
| timeout = 60 |
There was a problem hiding this comment.
Missing pytest-timeout dependency.
The timeout = 60 option requires the pytest-timeout plugin, which is not listed in the dev dependencies (only pytest and pytest-cov are included). Without it, this setting will be silently ignored.
🔧 Proposed fix
dev = [
"ovoscope[audio,pydantic]",
"pytest",
"pytest-cov",
+ "pytest-timeout",
]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pyproject.toml` at line 57, The pyproject setting "timeout = 60" relies on
the pytest-timeout plugin but that package is not listed alongside the existing
dev dependencies "pytest" and "pytest-cov"; add "pytest-timeout" to the
project's dev dependencies (the same section where "pytest" and "pytest-cov" are
declared) so the timeout option is honored by test runs, specifying an
appropriate version constraint.
| def _make_mock_client(messages_to_emit: List[Message]) -> MagicMock: | ||
| """Build a mock MessageBusClient that emits *messages_to_emit* when subscribed.""" | ||
| client = MagicMock() | ||
| client.connected_event = threading.Event() | ||
| client.connected_event.set() | ||
|
|
||
| handlers: dict = {} | ||
|
|
||
| def on_side_effect(event_type: str, handler: Any) -> None: | ||
| handlers.setdefault(event_type, []).append(handler) | ||
|
|
||
| def remove_side_effect(event_type: str, handler: Any) -> None: | ||
| if event_type in handlers: | ||
| try: | ||
| handlers[event_type].remove(handler) | ||
| except ValueError: | ||
| pass | ||
|
|
||
| def emit_side_effect(msg: Message) -> None: | ||
| # Deliver mocked response messages to subscribed handlers | ||
| for m in messages_to_emit: | ||
| for h in list(handlers.get("message", [])): | ||
| h(m.serialize()) | ||
|
|
||
| client.on.side_effect = on_side_effect | ||
| client.remove.side_effect = remove_side_effect | ||
| client.emit.side_effect = emit_side_effect |
There was a problem hiding this comment.
Make the mock client replay scripted responses only once.
emit_side_effect() pushes the full messages_to_emit sequence on every client.emit() call. If record() starts emitting an extra message or retrying internally, these tests still pass with duplicated responses instead of catching the regression.
💡 Minimal tightening
+ delivered = False
def emit_side_effect(msg: Message) -> None:
+ nonlocal delivered
+ if delivered:
+ return
+ delivered = True
# Deliver mocked response messages to subscribed handlers
for m in messages_to_emit:
for h in list(handlers.get("message", [])):
h(m.serialize())🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/unittests/test_remote_recorder.py` around lines 32 - 58, The mock's
emit_side_effect currently replays the entire messages_to_emit list on every
client.emit call, masking duplicate-emission bugs; modify _make_mock_client so
emit_side_effect consumes the scripted messages only once (e.g., capture
messages_to_emit into a local queue/iterator and on the first client.emit
deliver each message to handlers, then no-op on subsequent emits), keeping
references to handlers and using the same handler invocation logic as before.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ovoscope/__init__.py (1)
774-800:⚠️ Potential issue | 🟠 MajorMissing try/finally around bus coverage tracking —
bus.emitmay remain wrapped on failure.If
capture.capture()orcapture.finish()raises an exception,_bus_tracker.stop_tracking()is never called, leavingbus.emitwrapped for all subsequent tests using the sameMiniCroftinstance. This matches the concern raised in the past review.🛡️ Proposed fix: wrap tracking in try/finally
# bus coverage tracking (optional) _bus_tracker = None if self.track_bus_coverage: from ovoscope.bus_coverage import BusCoverageTracker _bus_tracker = BusCoverageTracker(self.minicroft.bus, self.minicroft) _bus_tracker.snapshot_listeners() _bus_tracker.start_tracking() - # the capture session will store all messages until capture.finish() - # even if multiple messages are emitted - capture = CaptureSession(self.minicroft, eof_msgs=self.eof_msgs, - ignore_messages=self.ignore_messages, - async_messages=self.async_messages) - for idx, source_message in enumerate(self.source_message): - if "session" not in source_message.context and len(capture.responses): - # propagate session updates as a client would do - source_message.context["session"] = capture.responses[-1].context["session"] - capture.capture(source_message, timeout) - - # final message list - messages = capture.finish() - - if _bus_tracker is not None: - _bus_tracker.stop_tracking() - all_responses = messages + list(getattr(capture, "async_responses", [])) - _bus_tracker.record_session(all_responses, self.expected_messages) - self.bus_coverage_report = _bus_tracker.build_report() + try: + # the capture session will store all messages until capture.finish() + # even if multiple messages are emitted + capture = CaptureSession(self.minicroft, eof_msgs=self.eof_msgs, + ignore_messages=self.ignore_messages, + async_messages=self.async_messages) + for idx, source_message in enumerate(self.source_message): + if "session" not in source_message.context and len(capture.responses): + # propagate session updates as a client would do + source_message.context["session"] = capture.responses[-1].context["session"] + capture.capture(source_message, timeout) + + # final message list + messages = capture.finish() + finally: + if _bus_tracker is not None: + _bus_tracker.stop_tracking() + + if _bus_tracker is not None: + all_responses = messages + list(getattr(capture, "async_responses", [])) + _bus_tracker.record_session(all_responses, self.expected_messages) + self.bus_coverage_report = _bus_tracker.build_report()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovoscope/__init__.py` around lines 774 - 800, Wrap the BusCoverageTracker usage in a try/finally so stop_tracking() always runs even if capture.capture() or capture.finish() raises: after creating _bus_tracker and starting it, run the capture loop and messages = capture.finish() inside a try block, then in finally call _bus_tracker.stop_tracking() if _bus_tracker is not None; after the try/finally (or inside finally if you need session data) call _bus_tracker.record_session(...) and set self.bus_coverage_report = _bus_tracker.build_report() only when _bus_tracker exists, and re-raise any exceptions so behavior is unchanged on error (ensure references: _bus_tracker, BusCoverageTracker, capture.capture, capture.finish, stop_tracking, record_session, build_report).
🧹 Nitpick comments (2)
ovoscope/pytest_plugin.py (1)
285-307: Code afteryieldin fixture executes after teardown, not afterfinally.Lines 285-307 run after the
finallyblock completes (afteryield collector). This code accessesrequest.configand writes files after the session fixture has already restored global state. While this works, it may cause confusion because:
- The merged report is built from
collector._reportswhich was populated during tests- Global state (
GLOBAL_BUS_COVERAGE, etc.) has already been reset toFalse/NoneConsider moving report finalization (filtering, file saving, attaching to config) inside the
tryblock beforeyield, or restructuring to clarify the lifecycle.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovoscope/pytest_plugin.py` around lines 285 - 307, The post-yield code that calls collector.merged_report(), filters it and writes the file executes after teardown (after GLOBAL_BUS_COVERAGE and other globals are reset), which is confusing; move the report finalization and file-write logic into the try block before yield (i.e., after collector has been populated but before yield collector) or otherwise restructure the fixture so that collector.merged_report(), report.filter(...), attaching to request.config._bus_coverage_reports and the cov_file write happen prior to yield, referencing the existing collector.merged_report(), request.config and cov_file handling so global state is still intact when building and saving the report.ovoscope/__init__.py (1)
150-228: Global monkey-patching at import time affects all FakeBus instances.
_patch_fakebus()is called unconditionally at module import (Line 227), modifyingFakeBus.on,.once,.emit, andOVOSSkill.add_event,.bindglobally. While the patches checkGLOBAL_BUS_COVERAGEbefore recording, the method dispatch overhead exists for all users.This is acceptable given the design goal of capturing boot-time events, but consider documenting this behavior prominently (e.g., in the module docstring or README) so users understand the import-time side effect.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovoscope/__init__.py` around lines 150 - 228, The module currently calls _patch_fakebus() at import-time which monkey-patches FakeBus.on/once/emit and OVOSSkill.add_event/bind globally and adds runtime dispatch overhead; update documentation to clearly state this import-time side-effect and its scope by adding a prominent note to the module-level docstring (near the top of ovoscope/__init__.py) and to the project README explaining that importing ovoscope will patch ovos_utils.fakebus and ovos_workshop globally, what GLOBAL_BUS_COVERAGE gates do, and any performance/compatibility implications so users are aware of the behavior.
🤖 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/bus-coverage.md`:
- Around line 143-151: Update the documentation's Source reference for
SkillBusCoverage: replace the incorrect `ovoscope/bus_coverage.py:118` with the
correct location for the SkillBusCoverage definition (lines 131–132) or remove
the specific line number and simply reference `ovoscope/bus_coverage.py` to
avoid future drift; ensure the symbol name `SkillBusCoverage` remains in the
Source line so readers can locate the class.
---
Outside diff comments:
In `@ovoscope/__init__.py`:
- Around line 774-800: Wrap the BusCoverageTracker usage in a try/finally so
stop_tracking() always runs even if capture.capture() or capture.finish()
raises: after creating _bus_tracker and starting it, run the capture loop and
messages = capture.finish() inside a try block, then in finally call
_bus_tracker.stop_tracking() if _bus_tracker is not None; after the try/finally
(or inside finally if you need session data) call
_bus_tracker.record_session(...) and set self.bus_coverage_report =
_bus_tracker.build_report() only when _bus_tracker exists, and re-raise any
exceptions so behavior is unchanged on error (ensure references: _bus_tracker,
BusCoverageTracker, capture.capture, capture.finish, stop_tracking,
record_session, build_report).
---
Nitpick comments:
In `@ovoscope/__init__.py`:
- Around line 150-228: The module currently calls _patch_fakebus() at
import-time which monkey-patches FakeBus.on/once/emit and
OVOSSkill.add_event/bind globally and adds runtime dispatch overhead; update
documentation to clearly state this import-time side-effect and its scope by
adding a prominent note to the module-level docstring (near the top of
ovoscope/__init__.py) and to the project README explaining that importing
ovoscope will patch ovos_utils.fakebus and ovos_workshop globally, what
GLOBAL_BUS_COVERAGE gates do, and any performance/compatibility implications so
users are aware of the behavior.
In `@ovoscope/pytest_plugin.py`:
- Around line 285-307: The post-yield code that calls collector.merged_report(),
filters it and writes the file executes after teardown (after
GLOBAL_BUS_COVERAGE and other globals are reset), which is confusing; move the
report finalization and file-write logic into the try block before yield (i.e.,
after collector has been populated but before yield collector) or otherwise
restructure the fixture so that collector.merged_report(), report.filter(...),
attaching to request.config._bus_coverage_reports and the cov_file write happen
prior to yield, referencing the existing collector.merged_report(),
request.config and cov_file handling so global state is still intact when
building and saving the report.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 77b35d06-be18-4d49-bbf6-1f204037c061
📒 Files selected for processing (8)
FAQ.mdMAINTENANCE_REPORT.mdSKILL.mddocs/bus-coverage.mddocs/gui-testing.mdovoscope/__init__.pyovoscope/bus_coverage.pyovoscope/pytest_plugin.py
✅ Files skipped from review due to trivial changes (1)
- SKILL.md
🚧 Files skipped from review as they are similar to previous changes (1)
- FAQ.md
| ## Calculations API | ||
|
|
||
| If you are building custom tooling, you can access these values via `SkillBusCoverage` properties: | ||
|
|
||
| * `listener_coverage_pct`: `(covered_listeners / total_listeners) * 100` | ||
| * `observed_emitter_pct`: `(observed_emitters / total_emitters) * 100` | ||
| * `asserted_emitter_pct`: `(asserted_emitters / total_emitters) * 100` | ||
|
|
||
| Source: `SkillBusCoverage` — `ovoscope/bus_coverage.py:118` |
There was a problem hiding this comment.
Minor: Incorrect source line reference.
The documentation states SkillBusCoverage is at ovoscope/bus_coverage.py:118, but looking at the source, SkillBusCoverage is defined at line 131-132.
Source: `SkillBusCoverage` — `ovoscope/bus_coverage.py:118`
+Source: `SkillBusCoverage` — `ovoscope/bus_coverage.py:131`📝 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.
| ## Calculations API | |
| If you are building custom tooling, you can access these values via `SkillBusCoverage` properties: | |
| * `listener_coverage_pct`: `(covered_listeners / total_listeners) * 100` | |
| * `observed_emitter_pct`: `(observed_emitters / total_emitters) * 100` | |
| * `asserted_emitter_pct`: `(asserted_emitters / total_emitters) * 100` | |
| Source: `SkillBusCoverage` — `ovoscope/bus_coverage.py:118` | |
| ## Calculations API | |
| If you are building custom tooling, you can access these values via `SkillBusCoverage` properties: | |
| * `listener_coverage_pct`: `(covered_listeners / total_listeners) * 100` | |
| * `observed_emitter_pct`: `(observed_emitters / total_emitters) * 100` | |
| * `asserted_emitter_pct`: `(asserted_emitters / total_emitters) * 100` | |
| Source: `SkillBusCoverage` — `ovoscope/bus_coverage.py:131` |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@docs/bus-coverage.md` around lines 143 - 151, Update the documentation's
Source reference for SkillBusCoverage: replace the incorrect
`ovoscope/bus_coverage.py:118` with the correct location for the
SkillBusCoverage definition (lines 131–132) or remove the specific line number
and simply reference `ovoscope/bus_coverage.py` to avoid future drift; ensure
the symbol name `SkillBusCoverage` remains in the Source line so readers can
locate the class.
- test_global_bus_coverage.py: Unit tests for global bus coverage tracking - test_gui_capture.py: Unit tests for GUI capture session These files were created locally but not committed to the PR branch. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/unittests/test_global_bus_coverage.py (1)
26-38: Consider resettingGLOBAL_BUS_COVERAGE_FILEfor completeness.The fixture resets
GLOBAL_BUS_COVERAGEandGLOBAL_BUS_COVERAGE_COLLECTOR, but per the global state defined inovoscope/__init__.py:104-147, there's alsoGLOBAL_BUS_COVERAGE_FILEthat could leak between tests if future tests modify it.🔧 Suggested improvement
def setup_globals(self): """Reset global state before and after each test.""" orig_enabled = ovoscope.GLOBAL_BUS_COVERAGE orig_collector = ovoscope.GLOBAL_BUS_COVERAGE_COLLECTOR + orig_file = ovoscope.GLOBAL_BUS_COVERAGE_FILE ovoscope.GLOBAL_BUS_COVERAGE = False ovoscope.GLOBAL_BUS_COVERAGE_COLLECTOR = None + ovoscope.GLOBAL_BUS_COVERAGE_FILE = None yield ovoscope.GLOBAL_BUS_COVERAGE = orig_enabled ovoscope.GLOBAL_BUS_COVERAGE_COLLECTOR = orig_collector + ovoscope.GLOBAL_BUS_COVERAGE_FILE = orig_file🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_global_bus_coverage.py` around lines 26 - 38, The fixture setup_globals should also save and restore ovoscope.GLOBAL_BUS_COVERAGE_FILE to avoid test leakage; capture the original value (orig_file = ovoscope.GLOBAL_BUS_COVERAGE_FILE) at the start, set ovoscope.GLOBAL_BUS_COVERAGE_FILE to None (or the appropriate neutral value) alongside GLOBAL_BUS_COVERAGE and GLOBAL_BUS_COVERAGE_COLLECTOR before yield, and restore ovoscope.GLOBAL_BUS_COVERAGE_FILE = orig_file after yield (referencing the setup_globals fixture and the GLOBAL_BUS_COVERAGE_FILE symbol).
🤖 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_global_bus_coverage.py`:
- Around line 26-38: The fixture setup_globals should also save and restore
ovoscope.GLOBAL_BUS_COVERAGE_FILE to avoid test leakage; capture the original
value (orig_file = ovoscope.GLOBAL_BUS_COVERAGE_FILE) at the start, set
ovoscope.GLOBAL_BUS_COVERAGE_FILE to None (or the appropriate neutral value)
alongside GLOBAL_BUS_COVERAGE and GLOBAL_BUS_COVERAGE_COLLECTOR before yield,
and restore ovoscope.GLOBAL_BUS_COVERAGE_FILE = orig_file after yield
(referencing the setup_globals fixture and the GLOBAL_BUS_COVERAGE_FILE symbol).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5b91a575-61b7-44ae-be84-365f177a62f7
📒 Files selected for processing (2)
test/unittests/test_global_bus_coverage.pytest/unittests/test_gui_capture.py
2 participants
Summary by CodeRabbit
New Features
Documentation
Tests
Chores