Conversation
- MiniCroft: added `lang` and `secondary_langs` params to patch Configuration() before IntentService init, enabling Adapt/Padatious to register vocab for multiple languages - End2EndTest.from_message(): fixed crash when async_messages=None - End2EndTest.from_message(): added ignore_gui param (default True) to filter GUI namespace messages during fixture recording - End2EndTest.from_message(): now passes ignore_messages, eof_msgs, async_messages to the returned End2EndTest object - Updated docs/minicroft.md with new constructor params and Multilingual Testing section Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
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:
📝 WalkthroughWalkthroughThis PR consolidates CI/CD workflows by delegating to external reusable workflows from OpenVoiceOS, adds language customization features to MiniCroft via new Changes
Sequence Diagram(s)sequenceDiagram
participant Test as Test Code
participant MiniCroft as MiniCroft.__init__
participant Config as global Configuration
participant SessionMgr as SessionManager
participant Restore as MiniCroft.stop()
Test->>MiniCroft: __init__(lang="es", secondary_langs=["fr"])
Note over MiniCroft: Store original values in _had_* flags
MiniCraft->>Config: Patch lang, secondary_langs<br/>(capture originals)
MiniCraft->>SessionMgr: Apply lang to<br/>default_session.lang
Note over MiniCraft: super().__init__() proceeds<br/>with patched config
Test->>Test: Run tests with<br/>language config applied
Test->>Restore: stop()
Restore->>Config: Restore original lang/<br/>secondary_langs or remove keys
Restore->>SessionMgr: Restore original lang setting
Note over Restore: Use _had_* flags to<br/>determine restore vs. removal
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (3)
test/unittests/test_pytest_plugin.py (1)
97-99: Prefix the unused patch argument if Ruff enforces ARG002.
mock_getis never read in this test, so Ruff will still flag the injected argument. Renaming it to_mock_get(or switching to a context-manager patch) keeps the file lint-clean without changing the test behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_pytest_plugin.py` around lines 97 - 99, The injected patch argument mock_get in the test function test_get_minicroft_failure_no_name_error is unused and triggers Ruff ARG002; rename the parameter to _mock_get (or alternatively apply the patch as a context manager) so the patch on plugin_mod.get_minicroft (side_effect=TimeoutError("boom")) remains applied but the unused-argument lint error is suppressed.test/unittests/test_end2end_extended.py (2)
632-632: Consider iterable unpacking for list concatenation.Ruff suggests using iterable unpacking (
[*a, *b]) instead of concatenation (a + b). This appears in multiple locations (lines 632, 730, 770, 865, 899, 977).♻️ Example refactor
-ignore_messages=["ovos.skills.settings_changed"] + HANDLER_LIFECYCLE, +ignore_messages=["ovos.skills.settings_changed", *HANDLER_LIFECYCLE],Similarly for
DEFAULT_IGNORED + HANDLER_LIFECYCLE:-ignore_messages=DEFAULT_IGNORED + HANDLER_LIFECYCLE, +ignore_messages=[*DEFAULT_IGNORED, *HANDLER_LIFECYCLE],🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_end2end_extended.py` at line 632, Replace list concatenation with iterable unpacking for clarity and to satisfy Ruff: where code builds lists like ["ovos.skills.settings_changed"] + HANDLER_LIFECYCLE (and similar occurrences combining DEFAULT_IGNORED + HANDLER_LIFECYCLE), change them to use unpacking (e.g., [*"ovos.skills.settings_changed"? not literal—unpack the single-item list and the other iterable]) so that the constructed lists use [*first_iterable, *second_iterable]; update the occurrences that form ignore_messages and any other list constructions that currently use + with HANDLER_LIFECYCLE or DEFAULT_IGNORED to use [*..., *...] instead.
622-636: Consider extracting duplicated_base_flagshelper.The
_base_flagsmethod is nearly identical inTestRoutingInternals(lines 622-636) andTestActiveSkillsInternals(lines 760-774), differing only in thetest_routingdefault. This could be extracted to a shared utility or base class.♻️ Suggested refactor
Create a shared helper or base class:
def _base_test_flags(*, test_routing=False, test_active_skills=False, **overrides): defaults = dict( test_message_number=False, test_msg_type=False, test_msg_data=False, test_msg_context=False, test_routing=test_routing, test_active_skills=test_active_skills, test_final_session=False, test_async_messages=False, test_async_message_number=False, ignore_messages=["ovos.skills.settings_changed", *HANDLER_LIFECYCLE], verbose=False, ) defaults.update(overrides) return defaultsThen use it in both test classes:
# In TestRoutingInternals **_base_test_flags(test_routing=True) # In TestActiveSkillsInternals **_base_test_flags(test_active_skills=True)Also applies to: 760-774
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_end2end_extended.py` around lines 622 - 636, The _base_flags method is duplicated between TestRoutingInternals and TestActiveSkillsInternals (only differing by test_routing/defaults), so extract a shared helper (e.g., _base_test_flags) that accepts explicit test_routing and test_active_skills kwargs and returns the same defaults dict (including ignore_messages built from HANDLER_LIFECYCLE); then replace the class-local _base_flags in both tests to call the new helper (pass test_routing=True in TestRoutingInternals, test_active_skills=True in TestActiveSkillsInternals) and update any callers to unpack the returned dict as before.
🤖 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/release_workflow.yml:
- Line 4: The workflow_dispatch currently allows manual runs from any branch
which can run jobs against non-dev refs; update the job-level conditions for the
build_tests and publish_alpha jobs to require github.ref == 'refs/heads/dev' in
addition to existing checks so manual triggers only run on dev; locate the jobs
named build_tests and publish_alpha in the workflow and add the branch guard
(e.g., && github.ref == 'refs/heads/dev') to their if expressions.
In `@FAQ.md`:
- Around line 40-42: The FAQ claim "104 tests across 6 test files" is
inconsistent with the documented "58-test baseline" and "52 new tests";
recalculate and reconcile the totals by running the test suite (pytest) to get
the authoritative test count and coverage, then update the "What test coverage
does ovoscope have?" paragraph to show the correct total and a clear breakdown
(baseline vs new tests and number of test files), replacing the "104 tests"
figure and ensuring the breakdown matches the pytest output and other PR
documentation (references: the strings "104 tests", "58-test baseline", and "52
new tests").
- Around line 34-35: The FAQ overstates the gate: update the FAQ.md line that
references release_workflow.yml so it accurately says the release workflow runs
the unit tests only (the build_tests job runs pytest test/unittests/) rather
than the “full test suite”; alternatively, if you intend to gate publish_alpha
on more than unit tests, modify the .github/workflows/release_workflow.yml
build_tests job to run the additional test suites (extend the pytest invocation
or add steps) and keep the FAQ wording as “full test suite” and ensure
publish_alpha still has needs: build_tests.
In `@ovoscope/__init__.py`:
- Line 641: from_message() currently uses ignore_gui while capturing but
constructs the End2EndTest without that flag, causing GUI messages to be
stripped or duplicated in __post_init__; modify from_message() (and the other
construction sites around the mentioned blocks) to preserve the capture-time
ignore_gui value by passing it into the End2EndTest constructor (e.g., add
ignore_gui=<captured flag> when building the recorded test) and ensure
__post_init__ uses the instance attribute rather than a global/local capture
list; keep the separate capture-time ignore list for deciding what to record,
but set the End2EndTest.ignore_gui attribute so replay logic respects the
original capture flag.
- Line 96: The teardown logic only restores keys when the stored _original_*
value is not None, which misses the case where the config key didn't exist
originally; change the override code in MiniCroft (where it sets
_original_secondary_langs, and the similar _original_* variables for other keys)
to also record a boolean flag (e.g. _had_secondary_langs) indicating whether
cfg.get("secondary_langs") existed, and in stop() use that flag to either
pop("secondary_langs") when it did not exist originally or restore the original
value when it did; apply the same pattern to the other override/restore pairs
referenced (_original_primary_langs, _original_lang, etc.) so unset keys are
removed instead of left mutated.
---
Nitpick comments:
In `@test/unittests/test_end2end_extended.py`:
- Line 632: Replace list concatenation with iterable unpacking for clarity and
to satisfy Ruff: where code builds lists like ["ovos.skills.settings_changed"] +
HANDLER_LIFECYCLE (and similar occurrences combining DEFAULT_IGNORED +
HANDLER_LIFECYCLE), change them to use unpacking (e.g.,
[*"ovos.skills.settings_changed"? not literal—unpack the single-item list and
the other iterable]) so that the constructed lists use [*first_iterable,
*second_iterable]; update the occurrences that form ignore_messages and any
other list constructions that currently use + with HANDLER_LIFECYCLE or
DEFAULT_IGNORED to use [*..., *...] instead.
- Around line 622-636: The _base_flags method is duplicated between
TestRoutingInternals and TestActiveSkillsInternals (only differing by
test_routing/defaults), so extract a shared helper (e.g., _base_test_flags) that
accepts explicit test_routing and test_active_skills kwargs and returns the same
defaults dict (including ignore_messages built from HANDLER_LIFECYCLE); then
replace the class-local _base_flags in both tests to call the new helper (pass
test_routing=True in TestRoutingInternals, test_active_skills=True in
TestActiveSkillsInternals) and update any callers to unpack the returned dict as
before.
In `@test/unittests/test_pytest_plugin.py`:
- Around line 97-99: The injected patch argument mock_get in the test function
test_get_minicroft_failure_no_name_error is unused and triggers Ruff ARG002;
rename the parameter to _mock_get (or alternatively apply the patch as a context
manager) so the patch on plugin_mod.get_minicroft
(side_effect=TimeoutError("boom")) remains applied but the unused-argument lint
error is suppressed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 45718737-6dd3-48b6-9878-bc10f4776ee9
⛔ Files ignored due to path filters (5)
test/__pycache__/__init__.cpython-311.pycis excluded by!**/*.pyctest/unittests/__pycache__/__init__.cpython-311.pycis excluded by!**/*.pyctest/unittests/__pycache__/test_capture_session.cpython-311-pytest-9.0.2.pycis excluded by!**/*.pyctest/unittests/__pycache__/test_end2end.cpython-311-pytest-9.0.2.pycis excluded by!**/*.pyctest/unittests/__pycache__/test_minicroft.cpython-311-pytest-9.0.2.pycis excluded by!**/*.pyc
📒 Files selected for processing (9)
.github/workflows/publish_stable.yml.github/workflows/release_workflow.ymlFAQ.mdMAINTENANCE_REPORT.mddocs/ci-integration.mddocs/minicroft.mdovoscope/__init__.pytest/unittests/test_end2end_extended.pytest/unittests/test_pytest_plugin.py
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
docs/ci-integration.md (2)
245-245: Avoid hard-coding the current test count.Line 245 will go stale as soon as tests are added or removed. I'd describe the workflow behavior without
58 tests, or link readers to the workflow file for live details.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/ci-integration.md` at line 245, Update the "Unit Tests" table entry so it doesn't hard-code the test count: remove "58 tests" and instead describe the behavior generically (e.g., "runs pytest with coverage") and/or point readers to the `unit_tests.yml` workflow for the current test count and details; ensure the row still mentions `unit_tests.yml` and the `pytest --cov=ovoscope` command so readers can find the live source of truth.
153-153: Pin the workflow reference to an immutable commit SHA instead of the@devbranch.Line 153 documents
@dev, a mutable branch reference that can be updated unexpectedly. GitHub recommends pinning reusable workflows to a full-length commit SHA for reproducibility and security, as branch names (and tags) can be moved and may unexpectedly pick up unintended changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/ci-integration.md` at line 153, The docs currently reference the reusable workflow as OpenVoiceOS/gh-automations/.github/workflows/publish-alpha.yml@dev which pins to a mutable branch; update that reference to use the full commit SHA for the target commit (e.g., replace `@dev` with @<full-commit-sha>) so the workflow is immutable and reproducible, ensuring the text shows the exact 40-character commit hash rather than a branch or tag..github/workflows/license_tests.yml (1)
13-15: Prefer an immutable ref for this reusable workflow.Using
@devmeans license results can change whenever the upstream branch moves, even for the same commit in this repo. Pinning to a release tag or commit SHA will make this check reproducible.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/license_tests.yml around lines 13 - 15, The workflow currently references the reusable workflow with a moving ref "OpenVoiceOS/gh-automations/.github/workflows/license-check.yml@dev"; replace that mutable ref with an immutable one (a release tag like `@v1.2.3` or a commit SHA) so results are reproducible—edit the uses line to e.g. "OpenVoiceOS/gh-automations/.github/workflows/license-check.yml@<TAG-or-SHA>" and keep the existing with: install_extras: '[pydantic]' unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/build_tests.yml:
- Around line 9-11: The workflow contains a dead PR filter 'requirements/**'
that doesn't exist; update the paths list so dependency changes actually trigger
the job by replacing 'requirements/**' with the repository's real dependency
files (for example add 'requirements.txt' or 'pyproject.toml' already present,
and/or 'poetry.lock' or 'setup.cfg' if those exist), or remove the nonexistent
pattern altogether and ensure the paths array includes the correct dependency
filenames referenced by your project.
- Around line 30-38: The workflow currently runs python -m build twice and then
installs from source using the "Install package" step (pip install .[pydantic]),
which bypasses validation of the built artifacts; remove the duplicate "Build
Source Packages"/"Build Distribution Packages" redundancy (keep a single python
-m build) and change the "Install package" step to install the actual built
wheel from dist (use pip install dist/*.whl[pydantic]) so the job verifies the
produced wheel/sdist rather than the source tree.
In @.github/workflows/pipaudit.yml:
- Around line 2-7: The workflow currently triggers only on push to master/dev
and manual dispatch; add a pull_request trigger to run the audit for incoming
PRs (at least for branch: dev) so dependency scans run before merge; update the
workflow "on" section to include a pull_request key with branches: - dev (and
optionally master) to ensure jobs defined in this file execute for PR events,
referencing the existing top-level on block in .github/workflows/pipaudit.yml.
- Around line 25-31: The workflow currently installs the package without extras
("pip install .") so the declared optional dependency group pydantic (from
pyproject.toml) isn't audited; update the GitHub Actions step named "Install
package" to install the pydantic extra (use pip install ".[pydantic]") so the
pypa/gh-action-pip-audit scan covers the pydantic extra dependency set
referenced in pyproject.toml.
In @.github/workflows/release_workflow.yml:
- Around line 29-39: The workflow currently references the upstream reusable
workflow with a moving ref
"OpenVoiceOS/gh-automations/.github/workflows/publish-alpha.yml@dev"; replace
that `@dev` ref with an immutable tag or full commit SHA (e.g., change the "uses"
value to ".../publish-alpha.yml@vX.Y.Z" or "...@<commit-sha>") so repeated runs
use the exact same upstream code; keep the rest of the block (secrets: inherit
and with: keys like branch, version_file, publish_pypi, etc.) unchanged.
In `@MAINTENANCE_REPORT.md`:
- Around line 35-39: Summary: Capitalize "GitHub" throughout the workflow notes
in MAINTENANCE_REPORT.md. Fix: update the text in the block that mentions
`.github/workflows/unit_tests.yml`, `.github/workflows/build_tests.yml`,
`.github/workflows/license_tests.yml`, `.github/workflows/pipaudit.yml`, and
`.github/workflows/release_workflow.yml` so every occurrence of "github" is
changed to "GitHub" (preserve surrounding punctuation and filenames exactly).
- Around line 1-28: The summary test-total math is inconsistent: the text lists
previous 58 tests and adds 46 + 6 = 52 new tests but still reports "58 → 104
tests"; update the summary and any matching totals to reflect 58 + 52 = 110
(change the "58 → 104 tests" occurrence to "58 → 110 tests" and verify other
places in MAINTENANCE_REPORT.md that mention totals or the two new files
`test/unittests/test_end2end_extended.py` and
`test/unittests/test_pytest_plugin.py` are consistent with 52 added tests and
the final total of 110).
---
Nitpick comments:
In @.github/workflows/license_tests.yml:
- Around line 13-15: The workflow currently references the reusable workflow
with a moving ref
"OpenVoiceOS/gh-automations/.github/workflows/license-check.yml@dev"; replace
that mutable ref with an immutable one (a release tag like `@v1.2.3` or a commit
SHA) so results are reproducible—edit the uses line to e.g.
"OpenVoiceOS/gh-automations/.github/workflows/license-check.yml@<TAG-or-SHA>"
and keep the existing with: install_extras: '[pydantic]' unchanged.
In `@docs/ci-integration.md`:
- Line 245: Update the "Unit Tests" table entry so it doesn't hard-code the test
count: remove "58 tests" and instead describe the behavior generically (e.g.,
"runs pytest with coverage") and/or point readers to the `unit_tests.yml`
workflow for the current test count and details; ensure the row still mentions
`unit_tests.yml` and the `pytest --cov=ovoscope` command so readers can find the
live source of truth.
- Line 153: The docs currently reference the reusable workflow as
OpenVoiceOS/gh-automations/.github/workflows/publish-alpha.yml@dev which pins to
a mutable branch; update that reference to use the full commit SHA for the
target commit (e.g., replace `@dev` with @<full-commit-sha>) so the workflow is
immutable and reproducible, ensuring the text shows the exact 40-character
commit hash rather than a branch or tag.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 57278595-9c58-4e41-9bf7-e9a6a4bf3644
📒 Files selected for processing (8)
.github/workflows/build_tests.yml.github/workflows/license_tests.yml.github/workflows/pipaudit.yml.github/workflows/publish_stable.yml.github/workflows/release_workflow.yml.github/workflows/unit_tests.ymlMAINTENANCE_REPORT.mddocs/ci-integration.md
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/publish_stable.yml
JarbasAl
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
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)
680-684:⚠️ Potential issue | 🟠 MajorHandle the first recorded message without a session.
On the first iteration,
capture.responsesis still empty, so Line 683 raisesIndexErrorwhenever the initial message lacks asessioncontext. Only copy from the previous response after the first message; otherwise seed from the current default session.Suggested fix
for idx, source_message in enumerate(message): if "session" not in source_message.context: # propagate session updates as a client would do - source_message.context["session"] = capture.responses[-1].context["session"] + if idx == 0 or not capture.responses: + source_message.context["session"] = ( + SessionManager.default_session.serialize() + ) + else: + source_message.context["session"] = ( + capture.responses[-1].context["session"] + ) capture.capture(source_message, timeout)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovoscope/__init__.py` around lines 680 - 684, When iterating messages, avoid indexing capture.responses[ -1 ] when it's empty; update the loop in which source_message without a "session" gets one by first checking if capture.responses is non-empty and only then copying capture.responses[-1].context["session"], otherwise seed source_message.context["session"] from the capture object's default/initial session (e.g., capture.default_session or capture.initial_session) before calling capture.capture(source_message, timeout); this ensures the first recorded message is given a valid session without raising IndexError.
🧹 Nitpick comments (1)
.github/workflows/release_workflow.yml (1)
19-24: Consider removing unusedbuildpackage and uploading coverage artifacts.
- The
buildpackage is installed but not used in this job.- Coverage XML is generated but not uploaded, so it's lost after the job completes.
♻️ Optional: Remove unused dependency and add coverage upload
- name: Install dependencies run: | - pip install build pytest pytest-cov + pip install pytest pytest-cov pip install -e .[pydantic] - name: Run unit tests run: pytest test/unittests/ -v --cov=ovoscope --cov-report xml + - name: Upload coverage report + uses: actions/upload-artifact@v4 + with: + name: coverage-report + path: coverage.xml🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release_workflow.yml around lines 19 - 24, Remove the unused "build" package from the "Install dependencies" step (change the pip install line that currently installs "build pytest pytest-cov" to only install the packages actually needed, e.g., "pytest pytest-cov" and the editable install), and add a new job step after the "Run unit tests" step to upload the generated coverage XML (coverage.xml) as a workflow artifact using the GitHub Actions upload-artifact action so the --cov-report xml output is preserved for later inspection.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ovoscope/__init__.py`:
- Around line 211-212: Before you overwrite SessionManager.default_session.lang
with self._lang, capture the current value from
SessionManager.default_session.lang into self._original_lang (not from
Configuration()); then in stop() restore SessionManager.default_session.lang
from that saved self._original_lang. Update the code paths that set/restore the
locale (the place assigning SessionManager.default_session.lang = self._lang and
the stop() restoration logic around lines 265-272) so they use the captured
original value to avoid leaking a config-derived locale.
- Around line 667-672: The function from_message currently uses `or` to assign
defaults (e.g., `eof_msgs = eof_msgs or DEFAULT_EOF`) which treats an explicit
empty list as falsy and overrides caller intent; change each defaulting
assignment to check for None (e.g., `if eof_msgs is None: eof_msgs =
DEFAULT_EOF`) for eof_msgs, flip_points, ignore_messages, and async_messages so
empty lists are preserved, and keep the ignore_gui branch to extend
ignore_messages only after you ensure ignore_messages is not None (or initialize
it from DEFAULT_IGNORED when None) so the GUI_IGNORED merge behaves correctly.
- Around line 421-424: The code mutates the shared default list by appending
GUI_IGNORED into self.ignore_messages which may be the module-level
DEFAULT_IGNORED; to fix, ensure self.ignore_messages is a new local list before
modifying it (e.g., copy it or create a new list/set) and then add GUI_IGNORED
entries (or perform a set-union) so GUI_IGNORED is not appended into the shared
DEFAULT_IGNORED; update the logic around GUI_IGNORED and self.ignore_messages to
work on that copied list and avoid in-place mutation.
---
Outside diff comments:
In `@ovoscope/__init__.py`:
- Around line 680-684: When iterating messages, avoid indexing
capture.responses[ -1 ] when it's empty; update the loop in which source_message
without a "session" gets one by first checking if capture.responses is non-empty
and only then copying capture.responses[-1].context["session"], otherwise seed
source_message.context["session"] from the capture object's default/initial
session (e.g., capture.default_session or capture.initial_session) before
calling capture.capture(source_message, timeout); this ensures the first
recorded message is given a valid session without raising IndexError.
---
Nitpick comments:
In @.github/workflows/release_workflow.yml:
- Around line 19-24: Remove the unused "build" package from the "Install
dependencies" step (change the pip install line that currently installs "build
pytest pytest-cov" to only install the packages actually needed, e.g., "pytest
pytest-cov" and the editable install), and add a new job step after the "Run
unit tests" step to upload the generated coverage XML (coverage.xml) as a
workflow artifact using the GitHub Actions upload-artifact action so the
--cov-report xml output is preserved for later inspection.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7ff55774-7604-4cdc-9d38-3efab658be83
📒 Files selected for processing (3)
.github/workflows/build_tests.yml.github/workflows/release_workflow.ymlovoscope/__init__.py
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/build_tests.yml
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/unit_tests.yml:
- Around line 6-24: The workflow currently ignores changes to '.github/**' in
both the push and pull_request `paths-ignore` lists, which causes CI to skip
when only workflow/CI files change; remove the '.github/**' entry from the
`paths-ignore` arrays (both the top-level and the push block) so updates to
workflow files trigger the job and the workflow validates its own CI changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4423441d-0113-4fc2-bcd8-e81eee44d081
📒 Files selected for processing (3)
.github/workflows/pipaudit.yml.github/workflows/unit_tests.ymlpyproject.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/pipaudit.yml
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
.github/workflows/release_workflow.yml (1)
29-29:⚠️ Potential issue | 🟠 MajorPin the publish workflow to an immutable ref.
A release path should not depend on
@dev; two runs for the same commit can execute different upstream publish logic. Use a tag or full commit SHA instead.Does GitHub recommend pinning third-party actions and reusable workflows to a full commit SHA instead of a branch ref, and does that guidance apply to release workflows?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/release_workflow.yml at line 29, The workflow currently references a reusable workflow with a mutable ref ("uses: OpenVoiceOS/gh-automations/.github/workflows/publish-alpha.yml@dev"), which can change between runs; update that line to pin to an immutable ref (a tag or full commit SHA) such as replacing "@dev" with a specific tag or the repository commit SHA for publish-alpha.yml so the release workflow always runs the exact same upstream code..github/workflows/build_tests.yml (1)
29-37:⚠️ Potential issue | 🟠 MajorBuild once, then install the artifact you just produced.
python -m buildalready creates both sdist and wheel, so the second build is redundant. More importantly,pip install .[pydantic]re-tests the source checkout instead of the wheel that would be published, which misses packaging and file-inclusion regressions. As per coding guidelines, "External-call hazards that can corrupt data or cause cascading outages" and "Reliability concerns that degrade user experience or resilience" should be fixed before release.Suggested change
- - name: Build Source Packages - run: | - python -m build - - name: Build Distribution Packages - run: | - python -m build - - name: Install package - run: | - pip install .[pydantic] + - name: Build packages + run: | + python -m build + - name: Install built wheel + run: | + WHEEL="$(echo dist/*.whl)" + python -m pip install "ovoscope[pydantic] @ file://${PWD}/${WHEEL}"What is the correct pip/PEP 508 syntax to install extras from a local wheel file, e.g. `package[extra] @ file:///.../dist/pkg.whl`?🤖 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 29 - 37, The workflow runs python -m build twice and then pip installs the source tree instead of the produced wheel; remove the duplicate Build Distribution Packages step, run build once, capture the produced .whl (e.g. WHEEL=$(ls dist/*.whl)) and install that wheel with extras using the PEP 508 file URL syntax like pip install "package[ pydantic] @ file://$PWD/$WHEEL" (or construct the correct package name/wheel path) so CI installs the actual artifact produced by the build rather than re-running a source install; update step names "Build Source Packages" / "Install package" accordingly.
🧹 Nitpick comments (1)
.github/workflows/license_check.yml (1)
13-13: Pin the reusable workflow to an immutable ref.
@devmakes this workflow a moving target—upstream changes can alter CI behavior without any PR in this repo. Use a release tag or full commit SHA instead. GitHub's security guidance recommends pinning third-party actions and reusable workflows to a full-length commit SHA for stability and supply-chain security. The same mutable-ref pattern appears in.github/workflows/pip_audit.yml,.github/workflows/release_preview.yml, and.github/workflows/python_support.yml.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/license_check.yml at line 13, The workflow currently references the reusable workflow with a mutable ref ("uses: OpenVoiceOS/gh-automations/.github/workflows/license-check.yml@dev"), which should be pinned to an immutable ref; update that uses line to point to a fixed release tag or full commit SHA (replace "@dev" with the chosen tag or full-length commit SHA) and make the same change in the other affected workflow files (.github/workflows/pip_audit.yml, .github/workflows/release_preview.yml, .github/workflows/python_support.yml) so CI consumes a stable, auditable version of the OpenVoiceOS/gh-automations workflow.
🤖 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 6-10: The pull_request.paths filter currently only lists
'pyproject.toml' which misses source and workflow changes; update the
pull_request.paths section in the build_tests workflow to also include the
package source and the workflow file itself by adding entries like 'ovoscope/**'
and '.github/workflows/build_tests.yml' (or remove the paths filter entirely) so
changes to package code or this workflow will trigger the CI; modify the
pull_request.paths block accordingly where it appears in the workflow (look for
the pull_request.paths key).
In @.github/workflows/repo_health.yml:
- Line 9: The reusable workflow reference currently uses a mutable branch tag
("uses: OpenVoiceOS/gh-automations/.github/workflows/repo-health.yml@dev");
replace the branch ref with an immutable full commit SHA for the desired version
of the OpenVoiceOS/gh-automations repo so CI is pinned to a specific commit
(update the "uses:
OpenVoiceOS/gh-automations/.github/workflows/repo-health.yml@..." entry to use
the full commit SHA string).
---
Duplicate comments:
In @.github/workflows/build_tests.yml:
- Around line 29-37: The workflow runs python -m build twice and then pip
installs the source tree instead of the produced wheel; remove the duplicate
Build Distribution Packages step, run build once, capture the produced .whl
(e.g. WHEEL=$(ls dist/*.whl)) and install that wheel with extras using the PEP
508 file URL syntax like pip install "package[ pydantic] @ file://$PWD/$WHEEL"
(or construct the correct package name/wheel path) so CI installs the actual
artifact produced by the build rather than re-running a source install; update
step names "Build Source Packages" / "Install package" accordingly.
In @.github/workflows/release_workflow.yml:
- Line 29: The workflow currently references a reusable workflow with a mutable
ref ("uses:
OpenVoiceOS/gh-automations/.github/workflows/publish-alpha.yml@dev"), which can
change between runs; update that line to pin to an immutable ref (a tag or full
commit SHA) such as replacing "@dev" with a specific tag or the repository
commit SHA for publish-alpha.yml so the release workflow always runs the exact
same upstream code.
---
Nitpick comments:
In @.github/workflows/license_check.yml:
- Line 13: The workflow currently references the reusable workflow with a
mutable ref ("uses:
OpenVoiceOS/gh-automations/.github/workflows/license-check.yml@dev"), which
should be pinned to an immutable ref; update that uses line to point to a fixed
release tag or full commit SHA (replace "@dev" with the chosen tag or
full-length commit SHA) and make the same change in the other affected workflow
files (.github/workflows/pip_audit.yml, .github/workflows/release_preview.yml,
.github/workflows/python_support.yml) so CI consumes a stable, auditable version
of the OpenVoiceOS/gh-automations workflow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cffdd830-630f-43ae-8625-0d6f6b896da0
📒 Files selected for processing (8)
.github/workflows/build_tests.yml.github/workflows/license_check.yml.github/workflows/pip_audit.yml.github/workflows/python_support.yml.github/workflows/release_preview.yml.github/workflows/release_workflow.yml.github/workflows/repo_health.yml.github/workflows/unit_tests.yml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/unit_tests.yml
- build_tests.yml: remove paths filter (now runs on all PRs); add package_name
and version_file for channel compatibility check
- release_preview.yml: add package_name and version_file for channel check
- unit_tests.yml: fix install_extras ("-e .[pydantic]" -> "[pydantic]");
remove push trigger and paths-ignore so coverage always posts on PRs
- release_workflow.yml: remove redundant github.ref guard on job conditions
- downstream_check.yml: new workflow
- python_support.yml: removed (superseded by build-tests.yml)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Hello! I've finished running some automated checks on this PR. 👋I've aggregated the results of the automated checks for this PR below. 🏷️ Release PreviewHere's what the next release might look like! 🚀 Current:
✅ PR title follows conventional commit format. 🚀 Release Channel Compatibility Predicted next version:
🔒 Security (pip-audit)Security check! Are we safe from vulnerabilities? 🛡️ ❌ Vulnerabilities found — check the job log for details. ⚖️ License CheckAre we all good on the legal front? Let's find out! 📑 ✅ No license violations found (60 packages). License distribution: 14× MIT, 14× MIT License, 9× Apache Software License, 4× Apache-2.0, 4× BSD-3-Clause, 2× ISC License (ISCL), 2× PSF-2.0, 2× Python Software Foundation License, +8 more Full breakdown — 60 packages
Copyright (c) 2022 Phil Ewels Permission is hereby granted, free of charge, to any person obtaining a copy The above copyright notice and this permission notice shall be included in all THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR Policy: Apache 2.0 (universal donor). StrongCopyleft / NetworkCopyleft / WeakCopyleft / Other / Error categories fail. MPL allowed. 🔨 Build TestsI've put your code through the build grinder. ☕ ✅ All versions pass
📋 Repo HealthHow's the repo's pulse? Let's take a look. 💓 ✅ All required files present. Latest Version: ✅ Automating the boring stuff so you don't have to! 🚀 |
build-tests.yml no longer uses these (channel check was removed from the reusable workflow). Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
- release_workflow.yml: replace inline build_tests job with reusable build-tests.yml@dev (runs full matrix 3.10-3.14, uses wheel install) - unit_tests.yml: fix install_extras — was '[pydantic]' (invalid pip arg), now 'ovoscope[pydantic]' which reinstalls with the optional extra after coverage.yml's editable install Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
JarbasAl
added a commit
that referenced
this pull request
Mar 10, 2026
* feat: docs, tests and some tiny improvements (#30) * feat: docs, tests and some tiny improvements * feat: docs, tests and some tiny improvements * feat: docs, tests and some tiny improvements * Increment Version to * Add renovate.json (#29) Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> * Increment Version to * Update Changelog * feat: Add language customization and expanded test coverage (#35) * feat: docs, tests and some tiny improvements * feat: docs, tests and some tiny improvements * feat: docs, tests and some tiny improvements * feat: add multilingual testing support and fix from_message bugs - MiniCroft: added `lang` and `secondary_langs` params to patch Configuration() before IntentService init, enabling Adapt/Padatious to register vocab for multiple languages - End2EndTest.from_message(): fixed crash when async_messages=None - End2EndTest.from_message(): added ignore_gui param (default True) to filter GUI namespace messages during fixture recording - End2EndTest.from_message(): now passes ignore_messages, eof_msgs, async_messages to the returned End2EndTest object - Updated docs/minicroft.md with new constructor params and Multilingual Testing section Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * chore: tests * chore: tests * chore: tests * coderabbitt * tests * tests * update * coderrabbit * coderrabbit * . * ci: update workflows for gh-automations refactor - build_tests.yml: remove paths filter (now runs on all PRs); add package_name and version_file for channel compatibility check - release_preview.yml: add package_name and version_file for channel check - unit_tests.yml: fix install_extras ("-e .[pydantic]" -> "[pydantic]"); remove push trigger and paths-ignore so coverage always posts on PRs - release_workflow.yml: remove redundant github.ref guard on job conditions - downstream_check.yml: new workflow - python_support.yml: removed (superseded by build-tests.yml) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * . * chore: remove unused package_name/version_file inputs from build_tests build-tests.yml no longer uses these (channel check was removed from the reusable workflow). Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com> * fix: standardize CI workflows for release readiness - release_workflow.yml: replace inline build_tests job with reusable build-tests.yml@dev (runs full matrix 3.10-3.14, uses wheel install) - unit_tests.yml: fix install_extras — was '[pydantic]' (invalid pip arg), now 'ovoscope[pydantic]' which reinstalls with the optional extra after coverage.yml's editable install Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * gitignore --------- Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> * Update downstream dependency report for ovoscope * Increment Version to 0.9.0a1 * Update Changelog * drop pycache * fix: clean up CI/CD — remove release gate, fix pydantic install (#36) * feat: docs, tests and some tiny improvements * feat: docs, tests and some tiny improvements * chore: tests * chore: tests * tests * tests * coderrabbit * ci: update workflows for gh-automations refactor - build_tests.yml: remove paths filter (now runs on all PRs); add package_name and version_file for channel compatibility check - release_preview.yml: add package_name and version_file for channel check - unit_tests.yml: fix install_extras ("-e .[pydantic]" -> "[pydantic]"); remove push trigger and paths-ignore so coverage always posts on PRs - release_workflow.yml: remove redundant github.ref guard on job conditions - downstream_check.yml: new workflow - python_support.yml: removed (superseded by build-tests.yml) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * chore: remove unused package_name/version_file inputs from build_tests build-tests.yml no longer uses these (channel check was removed from the reusable workflow). Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com> * fix: remove duplicate coverage_source line in unit_tests.yml Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix: split coverage into PR checks and Pages deployment - unit_tests.yml: coverage + PR comment on pull_request (permissions: pr write) - coverage_pages.yml: coverage + GitHub Pages on push to dev (permissions: pages, id-token) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> * cov --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> * Increment Version to 0.9.1a1 * Update Changelog * Increment Version to 0.9.1a2 --------- Co-authored-by: JarbasAI <33701864+JarbasAl@users.noreply.github.com> Co-authored-by: JarbasAl <JarbasAl@users.noreply.github.com> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: miro <jarbasai@mailfence.com>
This was referenced Mar 11, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Tests
Chores