Conversation
- pyproject.toml: Python project configuration (required for build) - AUDIT.md: Known issues and technical debt documentation - QUICK_FACTS.md: Machine-readable project reference - docs/: Architecture and feature documentation - .env: Environment configuration These files were merged but not committed. Required for CI builds to succeed. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
…overhead **Priority 1 — Race Conditions (correctness + perf)** - Add lock to _unload_plugin_skill to prevent concurrent dict mutation (skill_manager.py:585) - Snapshot plugin_skills dict inside lock for safe iteration in 4 methods: send_skill_list, deactivate_skill, activate_skill, deactivate_except Prevents RuntimeError: dictionary changed size during iteration - Replace busy-wait in _collect_fallback_skills with threading.Event signaling (fallback_service.py:122-125) — reduces CPU usage on utterances reaching fallback **Priority 2 — Per-Utterance Work (latency)** - Replace threading.Event().wait(1) with self._stop_event.wait(1) (skill_manager.py:462) — reuses event, correctly respects stop signal - Move migration_map dict and re.compile regex to module-level constants (service.py) — 15 utterances/second → rebuilding these on every pipeline stage - Guard create_daemon calls with config check before spawning thread (service.py:322, 352) — skip thread creation when open_data.intent_urls not configured **Priority 3 — Minor Overhead** - Change _logged_skill_warnings from list to set (O(1) lookup vs O(n)) (skill_manager.py:111) - Cache sorted plugins in all 3 transformer services (transformers.py) Invalidate cache on load_plugins() - Read blacklist once before plugin scan loop instead of per-skill (skill_manager.py:361) All 65 unit tests pass. Coverage maintained at 60% for ovos_core.skill_manager. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
…eport - FAQ.md: Add comprehensive Performance section documenting all optimizations (thread-safe loading, event signaling, caching, etc.) - MAINTENANCE_REPORT.md: Add detailed entry for 2026-03-11 performance optimization work - AUDIT.md: Document fixed race conditions (plugin_skills dict, busy-wait, temporary events) - SUGGESTIONS.md: Add S-007 marking all performance improvements as ADDRESSED All changes cross-referenced with code locations and commit SHA. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
|
Caution Review failedPull request was closed or merged during review 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:
📝 WalkthroughWalkthroughDocuments and fixes for race conditions and performance; refactors StopService to use OVOSAbstractApplication and changes match return types; adds GitHub-backed skill validation and pip uninstall; introduces lazy transformer caching; broad locale vocabulary updates; packaging/CI workflow additions; and extensive unit and end-to-end tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Utterance Handler
participant Intent as IntentService
participant Transformer as Transformer Service
participant Plugin as Pipeline Matcher
participant Session as SessionManager
Client->>Intent: handle_utterance(message)
Intent->>Transformer: _handle_transformers(utterances, context)
Transformer->>Transformer: compute/cache sorted plugins (_sorted_plugins)
Transformer-->>Intent: updated_message
Intent->>Intent: get_pipeline_matcher(plugin_id) (uses _PIPELINE_MIGRATION_MAP/_PIPELINE_RE)
Intent->>Plugin: match(utterances, lang)
alt Match Found
Plugin-->>Intent: IntentHandlerMatch
Intent->>Session: update_state/activate skill
Intent-->>Client: intent handled / utterance-handled
else No Match
Intent-->>Client: complete_intent_failure
end
sequenceDiagram
participant Service as FallbackService
participant Bus as Message Bus
participant Skill as Fallback Skill
participant Event as threading.Event
Service->>Bus: emit(skill.fallback.ping)
Bus->>Skill: ping
Service->>Event: clear()
loop wait cycle
Service->>Event: wait(timeout=0.02)
alt Event set
Skill->>Bus: emit(skill.fallback.pong, {can_handle: true})
Bus->>Service: handle_ack(msg)
Service->>Event: set()
Service->>Service: record skill
end
end
Service-->>Service: return collected_skills
sequenceDiagram
participant Installer as SkillInstaller
participant GitHub as GitHub API
Installer->>Installer: validate_skill(url)
Installer->>GitHub: GET /repos/{owner}/{repo}/contents (timeout=3s)
alt Network error / timeout
GitHub-->>Installer: error
Installer-->>Installer: return True (fail-open)
else 404
GitHub-->>Installer: 404
Installer-->>Installer: return False
else 200 with contents
GitHub-->>Installer: contents[]
Installer->>GitHub: GET pyproject.toml or setup.cfg or setup.py
alt packaging file references Mycroft/CommonPlay
Installer-->>Installer: return False
else acceptable packaging found
Installer-->>Installer: return True
end
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
**S-002: Implement skill uninstall (VALID FIX)** - Implement handle_uninstall_skill() to call pip_uninstall() for skill packages - Replace 'not implemented' error with actual uninstall logic - Convert skill_id to package name (dots → hyphens) - Validate skill parameter before attempting uninstall **Minor TODO clarifications** - Docker detection warning in launch_standalone() for container environments - Clarified voc_match() TODO: explain why StopService reimplements instead of using ovos_workshop (StopService is not a skill; voc_match is service-specific) **Test updates** - Updated test_handle_uninstall_skill to expect 'no packages to install' instead of 'not implemented' **S-006 (DEFERRED)** - Reverted external skills registry implementation - These TODOs are architectural limitations, not missing features - External skills run in separate processes and only communicate via messagebus - They cannot be listed, activated, or deactivated by ovos-core All 65 unit tests pass. Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
Beep! I'm back with the goodies! 🍭I've aggregated the results of the automated checks for this PR below. 🔒 Security (pip-audit)Looking for any weak links in the supply chain. ⛓️ ✅ No known vulnerabilities found (104 packages scanned). 🔨 Build TestsRunning the final assembly check. 🔧 ✅ All versions pass
⚖️ License CheckI've checked for any 'all rights reserved' surprises. 🛑 ❌ License violations detected (163 packages) — review required before merging. License distribution: 33× Apache Software License, 26× MIT License, 25× Apache-2.0, 20× MIT, 15× BSD License, 12× BSD-3-Clause, 3× apache-2.0, 2× GNU Lesser General Public License v3 or later (LGPLv3+), +22 more Full breakdown — 163 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 Copyright (c) 2016 Jannik Michelfeit 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. 📊 CoverageMapping the landscape of your tests. 🗺️ Files below 80% coverage (8 files)
Full report: download the 🔌 Skill Tests (ovoscope)Checking if the skill follows our conversational guidelines. 📏 ✅ 36/36 passed ✅ TestAdaptIntent — 4/4 🚌 Bus CoverageMeasuring the reach of our bus handlers. 📏 🔴 Coverage Summary
📊 Per-Skill Breakdown
🔍 Detailed Message Type Breakdown
|
| Signal | Value |
|---|---|
| Label | ignore-for-release |
| PR title | chore: docs tests and misc optimizations |
| Bump | alpha |
✅ PR title follows conventional commit format.
🚀 Release Channel Compatibility
Predicted next version: 2.1.4a2
| Channel | Status | Note | Current Constraint |
|---|---|---|---|
| Stable | ❌ | Too new (must be <1.4.0) | ovos-core>=1.3.1,<1.4.0 |
| Testing | ✅ | Compatible | ovos-core>=2.1.1,<3.0.0 |
| Alpha | ✅ | Compatible | ovos-core>=2.1.4a1 |
The pulse of the OpenVoiceOS codebase 💓
…itation - SUGGESTIONS.md: Mark S-002 as ADDRESSED with implementation details - SUGGESTIONS.md: Document S-006 as architectural limitation (external skills run in separate processes) - SUGGESTIONS.md: Explain correct pattern for external skills (self-advertise + respond to bus messages) - MAINTENANCE_REPORT.md: Add entry for S-002 implementation with rationale on S-006 - All references include commit SHAs and line numbers Co-Authored-By: Claude Haiku 4.5 <noreply@anthropic.com>
![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 (2)
ovos_core/intent_services/fallback_service.py (1)
43-43: Consider thread-safety for concurrent utterance processing.The
_fallback_response_eventis a single shared instance. If multiple utterances reach fallback processing concurrently, their event signals could interfere. This appears safe if_collect_fallback_skillsis called sequentially (one utterance at a time through the pipeline), but could cause subtle issues if the design ever changes to allow parallel utterance processing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovos_core/intent_services/fallback_service.py` at line 43, The single shared threading.Event instance _fallback_response_event is not thread-safe for concurrent utterance processing; remove the shared instance created in __init__ and instead create a per-utterance Event inside _collect_fallback_skills (or maintain a dict self._fallback_events keyed by a unique utterance/message id protected by a threading.Lock and cleaned up after use) and use that per-call Event wherever _fallback_response_event is referenced so signals for one utterance cannot affect another.ovos_core/skill_manager.py (1)
362-365: Convert the blacklist snapshot to aset.This path still does linear membership checks for every discovered plugin. Converting once here keeps the optimization local and makes the new warning-dedup path cheaper when many plugins are installed.
Proposed refactor
- blacklist = self.blacklist + blacklist = set(self.blacklist)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovos_core/skill_manager.py` around lines 362 - 365, Convert the snapshot of self.blacklist to a set before the loop to make membership checks O(1): replace the current assignment to blacklist with a set conversion (e.g., blacklist = set(self.blacklist)) and then keep the existing loop over plugins.items() and the check against self._logged_skill_warnings unchanged so the deduplication path benefits from faster lookups.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ovos_core/intent_services/service.py`:
- Around line 321-326: The guard that decides to call create_daemon currently
checks top-level Configuration for "open_data.intent_urls" while
_upload_match_data() reads open_data from the service instance config
initialized from Configuration().get("intents", {}); change the checks so they
read open_data from the same scope (use self.config.get("open_data", {}) rather
than Configuration().get("open_data", {})) before calling create_daemon for
_upload_match_data (and update the other similar check that schedules uploads
near the _upload_match_data call as well) so uploads are scheduled when the
intent-scoped open_data.intent_urls is set.
In `@ovos_core/skill_manager.py`:
- Around line 592-605: The code currently calls skill_loader.instance.shutdown()
and default_shutdown() while holding _plugin_skills_lock; change the flow to
minimize lock hold by acquiring _plugin_skills_lock, look up and remove (or
mark) the skill_loader from plugin_skills for skill_id, then release the lock
and call skill_loader.instance.shutdown() and
skill_loader.instance.default_shutdown() outside the lock; ensure you still
handle exceptions and use the same skill_loader and skill_id values captured
while under the lock so the shutdowns run on the correct object without holding
_plugin_skills_lock.
In `@ovos-hc.py`:
- Around line 15-17: The SERVICE_NAME variable is given a default so the runtime
check "if not SERVICE_NAME" in ovos-hc.py is dead; either remove the default
assignment (so SERVICE_NAME must come from the environment) or remove the
unreachable check block. Concretely, decide whether SERVICE_NAME should be
required: if required, stop assigning the default "voice" and read from
os.environ only so the existing guard remains meaningful; if the default is
intentional, delete the "if not SERVICE_NAME" print/exit block to eliminate dead
code. Ensure references to SERVICE_NAME are updated accordingly.
- Around line 30-42: The client connection is leaked because sys.exit() is
called before client.close(); wrap the readiness check so client.close() always
runs—either move client.close() to execute before any sys.exit() or (preferably)
put the call to client.wait_for_response(ready_msg) and the subsequent status
check inside a try/finally that calls client.close() in the finally block;
reference the existing client.wait_for_response(ready_msg), the printed
"Healthcheck OK"/"Healthcheck FAILED" branches, and the client.close() call to
locate where to add the try/finally or to relocate client.close().
In `@unnamed.patch`:
- Around line 50-55: The dependency constraint for ovos-plugin-manager currently
allows an alpha pre-release (">=2.1.0a1,<3.0.0"); update the version specifier
for ovos-plugin-manager to exclude pre-releases by changing it to
">=2.1.0,<3.0.0" so only stable 2.1.x+ releases are permitted.
---
Nitpick comments:
In `@ovos_core/intent_services/fallback_service.py`:
- Line 43: The single shared threading.Event instance _fallback_response_event
is not thread-safe for concurrent utterance processing; remove the shared
instance created in __init__ and instead create a per-utterance Event inside
_collect_fallback_skills (or maintain a dict self._fallback_events keyed by a
unique utterance/message id protected by a threading.Lock and cleaned up after
use) and use that per-call Event wherever _fallback_response_event is referenced
so signals for one utterance cannot affect another.
In `@ovos_core/skill_manager.py`:
- Around line 362-365: Convert the snapshot of self.blacklist to a set before
the loop to make membership checks O(1): replace the current assignment to
blacklist with a set conversion (e.g., blacklist = set(self.blacklist)) and then
keep the existing loop over plugins.items() and the check against
self._logged_skill_warnings unchanged so the deduplication path benefits from
faster lookups.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c188ffd3-29e2-4e6f-9935-9f39ffcf1c91
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
.coverageAUDIT.mdFAQ.mdMAINTENANCE_REPORT.mdSUGGESTIONS.mdovos-hc.pyovos_core/intent_services/fallback_service.pyovos_core/intent_services/service.pyovos_core/skill_manager.pyovos_core/transformers.pyunnamed.patch
…imeout - converse_service: wrap bus.on/event.wait/bus.remove in try/finally so the skill.converse.pong listener is always removed even if handle_ack raises; add skill_id guard against malformed pong; change can_handle default True→False (non-responding skill should not converse) - stop_service: same try/finally + skill_id guard + can_handle default True→False for skill.stop.pong listener - service.py: fix LOG.info string concat crash when cancel_word is None (use f-string instead of + operator) - skill_manager: add configurable max_wait to wait_for_intent_service (default 300 s via skills.intent_service_timeout); raises descriptive RuntimeError with instructions instead of looping forever Note: sound config caching was not applied — Configuration() is a live object in OVOS that reflects runtime changes without restart. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
StopService now follows the same pattern as CommonQAService and
OCPPipelineMatcher: double-inheriting ConfidenceMatcherPipeline and
OVOSAbstractApplication so that vocabulary loading and voc_match/voc_list
are provided by the shared ovos-workshop infrastructure instead of a
hand-rolled reimplementation.
Changes:
- Add OVOSAbstractApplication to base classes; call both __init__s with
skill_id="stop.openvoiceos" and resources_dir=dirname(__file__)
- Remove load_resource_files(), _voc_cache dict, _get_closest_lang(),
and the custom voc_match() override (~60 lines deleted)
- Replace self._voc_cache[lang]['stop'] in match_low with self.voc_list()
- Replace _get_closest_lang() guards in match_* with voc_list() emptiness
check (voc_list returns [] for unknown langs — no crash, no None sentinel)
- Rename all locale/*.intent files to *.voc so OVOSSkill resource loading
finds them via the standard ResourceType("vocab", ".voc", ...) path
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
27 tests covering: - _collect_stop_skills: no active skills, can_handle True/False, timeout cleanup (try/finally), exception cleanup, malformed pong guard (no skill_id), blacklisted skills excluded from ping - handle_stop_confirmation: error branch, response-mode abort_question, converse force_timeout, TTS stop when speaking, skill_id fallback from msg_type - match_high: no vocab → None, stop+no active skills → global stop, stop+active skills → skill stop, global_stop voc → global stop - match_medium: no voc → None, stop/global_stop voc delegates to match_low - match_low: empty voc_list → None, below threshold → None, active skill confidence boost, above threshold → skill stop - handle_global_stop / handle_skill_stop bus message forwarding - get_active_skills session delegation - shutdown removes both bus listeners Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…refactor
Fix existing stop e2e tests:
- Add 'stop.openvoiceos.stop.response' to all ignore_messages lists in
test_stop.py — StopService now subclasses OVOSAbstractApplication so it
responds to the mycroft.stop broadcast like other pipeline-plugin skills
(common_query, ocp, persona already filtered there)
New test file test_stop_refactor.py — 5 tests across 4 classes:
TestGlobalStopVocabulary (no skills loaded):
- test_global_stop_voc_no_active_skills: 'stop everything' matches
global_stop.voc and emits stop:global (regression: .voc rename works)
- test_stop_voc_exact_still_works: bare 'stop' still matches stop.voc
(regression: .voc rename did not break the stop vocabulary)
TestGlobalStopVocWithActiveSkill (count skill loaded):
- test_global_stop_voc_with_active_skill: 'stop everything now' emits
stop:global even when a skill is in the active list — verifying that
global_stop.voc takes priority over the stop:skill path
TestStopSkillCanHandleFalse (count skill loaded):
- test_stop_with_active_skill_ping_pong: full stop ping-pong sequence
with a running skill — verifies stop.ping → skill.stop.pong(can_handle=True)
→ stop:skill → {skill}.stop → {skill}.stop.response chain
TestStopServiceAsSkill (no skills loaded):
- test_stop_service_emits_activate_and_stop_response: explicitly asserts
that stop.openvoiceos.activate and stop.openvoiceos.stop.response appear
in the message sequence, confirming StopService participates in the
OVOSSkill stop lifecycle
Also installed missing test dependencies:
ovos-skill-count, ovos-skill-parrot, ovos-skill-hello-world,
ovos-skill-fallback-unknown, ovos-padatious-pipeline-plugin (from local
workspace), ovos-adapt-pipeline-plugin (from local workspace),
ovos-utterance-plugin-cancel (from local workspace)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Parse owner/repo from URL; call api.github.com/repos/{owner}/{repo}/contents/
- Reject repos that don't exist (HTTP 404)
- Reject bare setup.py-only repos (legacy Mycroft packaging)
- Fetch pyproject.toml/setup.cfg and reject if MycroftSkill or CommonPlaySkill found
- Fail-open on network errors and unexpected API status codes (3 s timeout)
- Fix 3 existing tests that assumed no network call (now mock requests.get/validate_skill)
- Add 10 new unit tests covering all validation branches
- Add test_converse_service.py (43 tests, 81% coverage)
- Update FAQ.md and MAINTENANCE_REPORT.md
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ervice
- test_fallback_service.py: 34 tests, 93% coverage
- handle_register/deregister_fallback, _fallback_allowed (ACCEPT_ALL/BLACKLIST/WHITELIST),
_collect_fallback_skills (ping-pong, timeouts, blacklisted sessions),
_fallback_range, match_high/medium/low delegation, shutdown
- test_transformers.py: 40 tests, 66% coverage
- All three transformer services (Utterance/Metadata/Intent)
- Plugin loading, priority ordering + caching, transform chaining,
exception swallowing, context merging, session key stripping
- test_intent_service_extended.py: 37 tests, raises service.py from 0% to 49%
- _handle_transformers, disambiguate_lang, get_pipeline_matcher (migration map),
get_pipeline, context handlers, send_cancel_event/send_complete_intent_failure,
_emit_match_message, handle_utterance (cancel/no-match), handle_get_intent, shutdown
Total: 247 unit tests passing
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
4 tests covering basic pipeline routing with ovos-skill-count: - Padatious intent matched end-to-end (full handler lifecycle) - High-priority pipeline stage handles before lower-priority stages - Unrecognized utterance produces complete_intent_failure + error sound - Blacklisted skill falls through to complete_intent_failure Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Regenerate all stop.voc and global_stop.voc files directly from
translations/{lang}/intents.json to ensure they stay in sync.
Changes:
- Preserve phrase order from translations (previously sorted alphabetically)
- Add missing phrases that existed in translations but not locale
- Remove phrases in locale that were not in translations
- Normalize fa-IR -> fa-ir (locale dir is always lowercase)
- nl-NL and nl-nl both exist in translations; nl-nl (canonical) wins
- nl-be has no global_stop translation — global_stop.voc intentionally absent
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Created .github/workflows/ovoscope.yml using gh-automations@dev reusable workflow - Enables bus coverage tracking for behavioural test metrics - Posts 🔌 Skill Tests (ovoscope) and 🚌 Bus Coverage sections to PR comments - Requires Adapt and Padatious pipelines for comprehensive intent testing - Updated FAQ.md with CI/Testing section explaining bus coverage - Updated QUICK_FACTS.md with testing workflow reference - Updated MAINTENANCE_REPORT.md with session log Bus coverage complements code coverage by showing which bus message types are exercised during tests, helping identify gaps in skill interaction testing. Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
There was a problem hiding this comment.
Actionable comments posted: 17
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ovos_core/intent_services/stop_service.py (1)
244-251:⚠️ Potential issue | 🟠 MajorFuzzy
global_stopphrases lose their semantics here.Once
match_medium()detectsglobal_stopwithexact=False, it hands off tomatch_low(), which only scores againststop.vocand then prefersstop:skillwhen active skills exist. A non-exact global-stop phrase can therefore stop only one skill—or fall through entirely—instead of emittingstop:global.Also applies to: 277-304
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovos_core/intent_services/stop_service.py` around lines 244 - 251, The current flow lets a fuzzy global_stop detected by voc_match(...) fall through to match_low(...) which only scores against stop.voc and biases stop:skill when active skills exist, losing the global_stop semantics; change the logic in the match_medium/match handler (the block using voc_match, get_active_skills, and calling match_low) so that when is_global_stop is true you either (A) short-circuit and return an explicit global-stop intent (e.g., the same structure match_low would return for stop:global) or (B) call match_low with an additional flag/param that forces matching against the global_stop vocabulary or forces preference for stop:global over stop:skill; update match_low signature/behavior accordingly so fuzzy global_stop phrases always map to stop:global, not to stop:skill or fall-through.
♻️ Duplicate comments (2)
ovos_core/skill_manager.py (1)
600-613:⚠️ Potential issue | 🟠 MajorRelease
_plugin_skills_lockbefore running skill shutdown hooks.
shutdown()anddefault_shutdown()execute arbitrary skill code. Holding the manager lock while they run can deadlock re-entrant teardown and stalls every otherplugin_skillsoperation.🔓 Suggested fix
- with self._plugin_skills_lock: - if skill_id in self.plugin_skills: - LOG.info('Unloading plugin skill: ' + skill_id) - skill_loader = self.plugin_skills[skill_id] - if skill_loader.instance is not None: - try: - skill_loader.instance.shutdown() - except Exception: - LOG.exception('Failed to run skill specific shutdown code: ' + skill_loader.skill_id) - try: - skill_loader.instance.default_shutdown() - except Exception: - LOG.exception('Failed to shutdown skill: ' + skill_loader.skill_id) - self.plugin_skills.pop(skill_id) + with self._plugin_skills_lock: + skill_loader = self.plugin_skills.pop(skill_id, None) + + if skill_loader is not None: + LOG.info('Unloading plugin skill: ' + skill_id) + if skill_loader.instance is not None: + try: + skill_loader.instance.shutdown() + except Exception: + LOG.exception('Failed to run skill specific shutdown code: ' + skill_loader.skill_id) + try: + skill_loader.instance.default_shutdown() + except Exception: + LOG.exception('Failed to shutdown skill: ' + skill_loader.skill_id)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovos_core/skill_manager.py` around lines 600 - 613, The code currently holds self._plugin_skills_lock while calling skill_loader.instance.shutdown() and default_shutdown(), risking deadlocks; change the flow in unload logic so you acquire the lock only to check and remove the SkillLoader from self.plugin_skills (e.g. get and pop skill_id into a local variable), then release the lock and call skill_loader.instance.shutdown() and skill_loader.instance.default_shutdown() outside the lock, preserving the same try/except logging behavior around those calls; keep references to _plugin_skills_lock, plugin_skills, skill_loader, shutdown and default_shutdown to locate and modify the block.ovos_core/intent_services/service.py (1)
321-326:⚠️ Potential issue | 🟠 MajorGate telemetry uploads from the same config scope used by the uploader.
These branches check
self.config.get("open_data", {}), but_upload_match_data()readsConfiguration().get("open_data", {}). With that mismatch, uploads silently stop depending on whereintent_urlsis configured.🛠️ Suggested fix
- if self.config.get("open_data", {}).get("intent_urls"): + if Configuration().get("open_data", {}).get("intent_urls"): create_daemon(self._upload_match_data, (match.utterance, match.match_type, lang, match.match_data)) @@ - if self.config.get("open_data", {}).get("intent_urls"): + if Configuration().get("open_data", {}).get("intent_urls"): create_daemon(self._upload_match_data, (match.utterance, "complete_intent_failure", lang, match.match_data))Also applies to: 352-357
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovos_core/intent_services/service.py` around lines 321 - 326, The telemetry upload is gated using self.config.get("open_data", {}) but _upload_match_data() reads Configuration().get("open_data", {}), causing a mismatch; fix by making the gating and uploader use the same config source—either modify _upload_match_data to read from self.config (preferred) or pass the resolved open_data into create_daemon when calling _upload_match_data (e.g., call create_daemon(self._upload_match_data, (open_data, match.utterance, ...))), and update the _upload_match_data signature accordingly so both checks reference the same config.
🧹 Nitpick comments (17)
ovos_core/intent_services/locale/uk-ua/global_stop.voc (1)
6-15: Consider addingусіvariants for recognition robustness.Optional: add parallel forms with
усі(in addition toвсі) to improve matching across speaker preference and ASR normalization behavior.Suggested additions
+припиніть усі процеси +припиніть усі операції +скасуйте усі завдання +скасуйте усі очікуючі операції +завершіть усі відкриті завдання +припиніть усі активні дії🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovos_core/intent_services/locale/uk-ua/global_stop.voc` around lines 6 - 15, Add parallel Ukrainian variants using "усі" alongside existing "всі" entries in the global_stop.voc vocabulary to improve ASR and speaker-variant matching; update the entries currently containing "всі" (e.g., "припиніть всі процеси", "припиніть всі операції", "скасуйте всі завдання", "завершіть всі дії", etc.) by adding corresponding lines with "усі" (e.g., "припиніть усі процеси", "припиніть усі операції", "скасуйте усі завдання", "завершіть усі дії", etc.) so each original phrase has a direct "усі" counterpart.ovos_core/intent_services/locale/gl-es/global_stop.voc (1)
8-16: Deduplicate repeated stop utterances in this block.Line 8 duplicates Line 1, Line 15 duplicates Line 10, and Line 16 duplicates Line 9. Removing exact duplicates keeps the vocab clean and avoids accidental weighting bias.
Proposed cleanup
parar todo rematar todo finalizar todo cancelar todo acabar todo interromper todo deter todo -parar todo paralo todo detelo todo finalizalo todo cancelalo todo acabalo todo interrompelo todo -detelo todo -paralo todo Parar todo agora🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovos_core/intent_services/locale/gl-es/global_stop.voc` around lines 8 - 16, The block contains exact duplicate stop utterances; remove the repeated lines so each phrase appears only once (specifically deduplicate "parar todo", "paralo todo", and "detelo todo" so only a single occurrence of each remains), ensure no extra leading/trailing whitespace or blank lines, and save the updated global_stop.voc with the remaining unique stop variants intact.ovos_core/intent_services/locale/ca-es/stop.voc (1)
13-14: Minor inconsistencies in capitalization and punctuation.
- Line 13:
(Atura|para)has inconsistent capitalization within the alternation group. Other lines use consistent casing like(Atura|Para)(lines 15-16) or(atura|para)(line 10).- Line 14: Missing comma after "Si us plau" — other polite forms (lines 8 and 11) include the comma:
Si us plau,.These won't affect voice recognition functionality (typically case-insensitive), but could be cleaned up for consistency.
✏️ Suggested consistency fix
-(Atura|para) la comanda actual -Si us plau (atura|para) la tasca actual +(Atura|Para) la comanda actual +Si us plau, (atura|para) la tasca actual🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovos_core/intent_services/locale/ca-es/stop.voc` around lines 13 - 14, Normalize the capitalization and punctuation in stop.voc: change the alternation token `(Atura|para)` to match the project's chosen casing (e.g., `(atura|para)` or `(Atura|Para)`) so it is consistent with other alternations, and add the missing comma after the polite phrase "Si us plau" so the line reads "Si us plau, (atura|para) la tasca actual" (adjust casing to the consistent style you choose).ovos_core/intent_services/locale/gl-es/stop.voc (1)
9-15: Remove duplicate vocabulary entry.
Parar o proceso en cursoappears twice (Line [9] and Line [15]). Keep one entry to avoid redundant weighting/noise.♻️ Proposed cleanup
Parar o proceso en curso ... -Parar o proceso en curso🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovos_core/intent_services/locale/gl-es/stop.voc` around lines 9 - 15, Remove the duplicated vocabulary entry "Parar o proceso en curso" in the stop.voc list so it appears only once; locate both occurrences of the exact phrase and delete the redundant line, leaving the rest of the entries and formatting intact.ovos_core/intent_services/locale/de-de/global_stop.voc (1)
4-5: Pattern on line 4 is redundant.Line 5
(breche|brech) (alles) abmakes "alles" optional, which means it already matches everything from line 4 ((breche|brech) alles ab). Line 4 can be removed unless the explicit pattern is intentionally kept for readability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovos_core/intent_services/locale/de-de/global_stop.voc` around lines 4 - 5, The pattern "(breche|brech) alles ab" is redundant because "(breche|brech) (alles) ab" already matches both forms; remove the explicit "(breche|brech) alles ab" entry from global_stop.voc (or keep it but add a comment explaining it's for readability) so only the singular pattern "(breche|brech) (alles) ab" remains to avoid duplicate matches.ovos_core/intent_services/locale/it-it/global_stop.voc (1)
29-31: Consider removing duplicate generated phrase at Line 30.Line 30 expands to
... tutte le attività in esecuzione, which is already present at Line 26. Keeping one source avoids duplicate training/matching weight on the same phrase.✂️ Optional dedupe tweak
-(Interrompi|termina|stoppa|ferma) tutte le (azioni|attività) in (esecuzione|corso) +(Interrompi|termina|stoppa|ferma) tutte le azioni in (esecuzione|corso) +(Interrompi|termina|stoppa|ferma) tutte le attività in corso🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovos_core/intent_services/locale/it-it/global_stop.voc` around lines 29 - 31, Remove the duplicate generated phrase by deleting or modifying the pattern "(Interrompi|termina|stoppa|ferma) tutte le (azioni|attività) in (esecuzione|corso)" so it no longer expands to the same utterance as the existing "(Interrompi|termina|stoppa|ferma) tutte le attività in esecuzione"; update the remaining patterns in global_stop.voc to ensure each line produces unique phrases and avoid redundant training/matching weight on identical sentences.ovos_core/intent_services/locale/fa-ir/stop.voc (1)
15-16: Remove duplicate phrase to keep the lexicon clean.
فعالیت فعلی رو بیخیال شوappears twice in this file (existing + changed). Keeping one copy is enough.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovos_core/intent_services/locale/fa-ir/stop.voc` around lines 15 - 16, The file contains a duplicated vocabulary entry "فعالیت فعلی رو بیخیال شو" in the stop.voc lexicon; remove the redundant occurrence so that only one instance of "فعالیت فعلی رو بیخیال شو" remains (keep the other unique entry "فعالیت فعلی رو متوقف کن"), ensuring the stop.voc vocabulary has no duplicate phrases.ovos_core/intent_services/locale/ca-es/global_stop.voc (1)
6-17: Deduplicate repeated stop phrases and equivalent patterns.This block repeats entries (e.g.,
acaba-ho tot,cessa tot) and equivalent regex forms ((-ho|)vs(|-ho)). Keep one canonical form per phrase to reduce lexicon noise.Proposed cleanup
- acaba-ho tot ... - cessa tot ... - avorta(|-ho) tot🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovos_core/intent_services/locale/ca-es/global_stop.voc` around lines 6 - 17, The list in global_stop.voc contains duplicate stop phrases and inconsistent optional-suffix patterns; deduplicate entries by keeping one canonical form per phrase (e.g., keep a single "acaba-ho tot" and a single "cessa tot") and normalize the optional "-ho" pattern across all lines (pick one consistent pattern instead of mixing "(-ho|)" and "(|-ho)"); update the block so each unique stop phrase appears only once and all optional-suffix variants use the same canonical regex form.ovos_core/intent_services/locale/eu/global_stop.voc (1)
28-31: Drop newly introduced duplicate entries.
gelditu dena,bukatu dena, andbukatu gauza guztiakare already present earlier in the file. Remove duplicates to keep the vocabulary set minimal.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovos_core/intent_services/locale/eu/global_stop.voc` around lines 28 - 31, Remove the duplicate vocabulary lines introduced—specifically drop the entries "gelditu dena", "bukatu dena", and "bukatu gauza guztiak" from the block that also contains "gelditu gauza guztiak" so only the original single occurrences remain; edit the file's vocabulary list in ovos_core/intent_services/locale/eu/global_stop.voc to delete those duplicate literal lines and leave the earlier unique entries intact.ovos_core/intent_services/locale/pl-pl/stop.voc (1)
8-9: Please deduplicate repeated Polish entries.
Zatrzymaj bieżące działanieandZakończ bieżące działanieare duplicated in this file. Keeping single instances will simplify maintenance.Also applies to: 16-17
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovos_core/intent_services/locale/pl-pl/stop.voc` around lines 8 - 9, Remove duplicate Polish entries so each phrase appears only once: locate all occurrences of "Zatrzymaj bieżące działanie" and "Zakończ bieżące działanie" and keep a single instance of each (remove the extras), and likewise deduplicate any repeated variants such as "Zatrzymaj bieżący proces" so the vocabulary file contains unique lines only; ensure quoting/spacing matches existing entries so intent matching is unchanged.ovos_core/intent_services/locale/da-dk/global_stop.voc (1)
1-16: Deduplicate repeated utterances in global_stop vocabulary.There are exact duplicates in this segment (for example
stop altandafslutte alt). Keeping only unique lines will reduce noise and simplify future maintenance.♻️ Suggested cleanup
stop alt afslut alt afslut alle annuller alle afslutte alle stands alt afbryd alt ophør med alt -stop alt afslutte alt -afslutte alt annullere alt -afslutte alt standse alt afbryde alt ophøre med alt🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovos_core/intent_services/locale/da-dk/global_stop.voc` around lines 1 - 16, The vocabulary contains exact duplicate utterances (e.g., "stop alt" and "afslutte alt") which should be deduplicated; edit the global_stop vocabulary block to remove repeated lines so each utterance appears only once, preserving one instance of each unique Danish phrase and keeping the file as a newline-separated list of unique utterances (check occurrences of "stop alt", "afslutte alt", "annuller alle" etc. and remove duplicates).ovos_core/intent_services/locale/nl-nl/global_stop.voc (1)
1-13: Deduplicate repeated NL-NL global stop lines.
stop allesis repeated several times in this changed segment. Keeping only unique lines will make the vocabulary cleaner and easier to maintain.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovos_core/intent_services/locale/nl-nl/global_stop.voc` around lines 1 - 13, The NL-NL global stop vocabulary contains repeated lines (notably "stop alles" and other duplicates); edit global_stop.voc to deduplicate entries so each utterance appears only once (preserve one occurrence of "stop alles", "beëindig alles", "alles stoppen", "alles annuleren", "alles afmaken", "alles afbreken", "alles beëindigen", etc.), resulting in a list of unique stop phrases to keep the vocabulary clean and maintainable.ovos_core/intent_services/locale/da-dk/stop.voc (1)
15-16: Remove duplicate phrase entry.Line 15 and Line 16 are identical (
Stop den aktuelle handling). One entry is enough.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovos_core/intent_services/locale/da-dk/stop.voc` around lines 15 - 16, Remove the duplicate phrase entry "Stop den aktuelle handling" from the da-dk stop.voc vocabulary file so the line appears only once; locate the two identical lines and delete one, ensuring the file contains unique utterances only.test/unittests/test_skill_installer.py (1)
293-299: Cover the uninstall happy path too.This now only exercises the empty-input branch. The new uninstall flow—normalizing
skill_id, callingpip_uninstall(), and emitting the complete/failed bus message—isn't asserted anywhere in this file, so the core path can regress unnoticed.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_skill_installer.py` around lines 293 - 299, Add a new assertion block to cover the successful uninstall path in test_handle_uninstall_skill: call skills_store.handle_uninstall_skill with a Message containing a valid skill id (ensuring the id will be normalized), mock skills_store.pip_uninstall to return success, assert pip_uninstall was called with the normalized skill id, assert no play_error_sound was triggered, and assert the bus emitted the success message type (e.g., "ovos.skills.uninstall.complete") with appropriate success data rather than the failure/error payload.test/unittests/test_transformers.py (1)
248-336: Add cache-clear coverage for the other transformer services.The lazy
_sorted_pluginscache was added across utterance, metadata, and intent transformers, but onlyUtteranceTransformersService.load_plugins()is checked for clearing it. Add the same regression test forMetadataTransformersServiceandIntentTransformersServiceso stale ordering bugs in those paths don't slip through.Also applies to: 352-451
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_transformers.py` around lines 248 - 336, Add tests mirroring the UtteranceTransformersService cache-clear check to cover the other services: set a sentinel on the service instance's _sorted_plugins, call MetadataTransformersService.load_plugins and IntentTransformersService.load_plugins, and assert the sentinel is cleared; use the same helper mocks/patches as existing tests (patch find_metadata_transformer_plugins / find_intent_transformer_plugins and Configuration) and add two new test methods (e.g. test_metadata_load_plugins_clears_sorted_cache and test_intent_load_plugins_clears_sorted_cache) in test_transformers.py that set svc._sorted_plugins to a non-None value, invoke load_plugins(), and assert svc._sorted_plugins is reset.test/end2end/test_intent_pipeline.py (1)
99-107: Avoid coupling these routing tests toovos-skill-countinternals.These cases are validating pipeline routing, but the
mycroft.skill.handler.{start,complete}assertions pin the exact Python method name emitted by an external skill. A harmless refactor insideovos-skill-count.openvoiceoscan fail this suite even when routing still works.Also applies to: 164-172
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/end2end/test_intent_pipeline.py` around lines 99 - 107, The tests currently assert exact external-skill internals by expecting Message events with data name "CountSkill.handle_how_are_you_intent" which couples the routing tests to ovos-skill-count implementation; change those assertions to check the generic routing/event identity instead (e.g., assert the "mycroft.skill.handler.start" and "mycroft.skill.handler.complete" messages are emitted and verify a non-implementation-specific identifier such as the intent name or skill_id in data/context rather than the Python method string), updating the Message checks in test_intent_pipeline.py (the blocks referencing "mycroft.skill.handler.start"/"mycroft.skill.handler.complete" and "CountSkill.handle_how_are_you_intent") to validate routing without the exact method name.test/end2end/test_stop_refactor.py (1)
196-205: Replace the fixed sleep with an event-based wait.If the background
"count to infinity"handler has not actually reached its running state before the stop utterance is sent, this test exercises the wrong path.time.sleep(2)is both flaky and slower than necessary; wait for a concrete bus event from the skill instead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/end2end/test_stop_refactor.py` around lines 196 - 205, The test currently uses time.sleep(2) after create_daemon(make_it_count), which is flaky; instead register a temporary listener on self.minicroft.bus that waits for the concrete bus event emitted by the skill when the background "count to infinity" handler reaches running (e.g., the skill-specific running/start event), use a threading.Event (or similar) that the listener sets, call create_daemon(make_it_count) then wait on that event with a short timeout, and only proceed to send the stop utterance once the event is set; reference make_it_count, create_daemon, self.minicroft.bus.emit, session and self.skill_id when locating where to add the listener and event handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ovos_core/intent_services/locale/de-de/global_stop.voc`:
- Around line 3-16: Remove duplicate vocabulary entries in this vocabulary file
by keeping a single instance of each unique phrase/pattern and deleting repeated
lines; specifically ensure only one occurrence remains for "beende alles",
"(breche|brech) alles ab", "(breche|brech) (alles) ab", "alles anhalten", "alles
abbrechen", "alles aufgeben", "alles stoppen" (preserve the regex variants as
written) so the file contains one canonical line per phrase/pattern and no
duplicate entries.
In `@ovos_core/intent_services/locale/es-es/global_stop.voc`:
- Line 2: The exact-match entry in global_stop.voc currently has a doubled space
("finalizar todo") which prevents matching when stop_service.py uses
exact=True; update the token to a single space ("finalizar todo") so the intent
matches correctly in the exact lookup used by stop_service.py.
In `@ovos_core/intent_services/locale/it-it/global_stop.voc`:
- Around line 3-25: This file global_stop.voc contains 23 literal `[UNUSED]`
entries which are interpreted by the ovos_workshop vocabulary loader and
expand_template() as optional tokens, producing both "unused" and an empty
utterance—remove or disable these lines by replacing each `[UNUSED]` with a real
stop phrase or prefixing the line with `#` to comment it out so the loader skips
them; ensure no remaining square-bracket placeholders remain in global_stop.voc
to avoid generating unintended stop triggers.
In `@ovos_core/intent_services/locale/it-it/stop.voc`:
- Around line 12-17: Remove all placeholder "[UNUSED]" tokens from the
production vocabulary file and replace them with real Italian stop utterances or
delete the lines entirely so the .voc contains only valid user utterances;
specifically search for the literal token "[UNUSED]" in stop.voc and remove
those entries, preserving file encoding/format and ensuring there are no blank
placeholder lines left behind.
In `@ovos_core/intent_services/locale/pt-br/global_stop.voc`:
- Around line 2-3: Remove the duplicate vocabulary entries in the pt-br
global_stop vocabulary: delete the repeated lines containing "termine
(todos|todas)" and the repeated lines containing "termine tudo" so each phrase
appears only once in ovos_core/intent_services/locale/pt-br/global_stop.voc;
ensure no other identical duplicate tokens remain in that file by scanning for
repeated exact lines and keeping a single instance of each unique phrase.
In `@ovos_core/intent_services/locale/pt-pt/global_stop.voc`:
- Line 21: Fix the typo in the voice pattern by replacing the incorrect token
"taredas" with "tarefas" in the pattern string "(termina|terminar|completar)
todas as taredas" so the locale entry matches "todas as tarefas"; update the
corresponding line in the pt-pt global_stop vocabulary file (the pattern shown)
to "(termina|terminar|completar) todas as tarefas".
In `@ovos_core/intent_services/locale/pt-pt/stop.voc`:
- Line 12: The stop phrase in stop.voc uses "(Pára|pare)o que estás a fazer"
which lacks the required space and, because stop.voc is matched with exact=True,
only matches the concatenated form; update the phrase to "(Pára|pare) o que
estás a fazer" (insert a space after the group) so the file matches the spoken
phrase correctly.
In `@ovos_core/intent_services/stop_service.py`:
- Around line 24-30: Resolve the bus once and pass the same instance to both
base class initializers: compute a single resolved_bus = bus or FakeBus() before
calling OVOSAbstractApplication.__init__ and ConfidenceMatcherPipeline.__init__,
then pass resolved_bus as the bus argument to both so self.bus is consistent and
listeners registered later (the ones attached after initialization) attach to
the same bus instance.
In `@ovos_core/skill_installer.py`:
- Around line 286-306: The current validate_skill block only fetches packaging
files (pyproject.toml/setup.cfg) and looks for legacy_class names, which misses
legacy references in actual Python modules; update validate_skill to follow the
package entry point or scan the repository's Python source instead: determine
the declared entry point from pyproject.toml/setup.cfg (or default package
layout), fetch the referenced module(s) or glob the repo for .py files, then
search those files' contents for "MycroftSkill" and "CommonPlaySkill" (the
legacy_class identifiers) and return False if found; keep the existing
network/error handling and timeouts used for manifest requests but apply them to
fetching the Python files or entry-point target instead.
- Around line 358-364: The handler currently lets exceptions from pip_uninstall
propagate, so wrap the call to pip_uninstall([pkg_name]) in a try/except that
catches subprocess errors (and general exceptions), and on exception call
LOG.error with the exception and emit the failure reply via
self.bus.emit(message.reply("ovos.skills.uninstall.failed", {"error":
InstallError.PIP_ERROR.value})); if pip_uninstall returns False still emit the
same failure reply, and only emit "ovos.skills.uninstall.complete" when
pip_uninstall returns truthy. Use the existing symbols pip_uninstall,
InstallError, self.bus.emit and message.reply to locate and update the logic.
In `@ovos_core/skill_manager.py`:
- Around line 456-471: In wait_for_intent_service(), elapsed is only incremented
by 1 even though each loop may block up to 5s in bus.wait_for_response and 1s in
_stop_event.wait, causing the max_wait check to be incorrect; measure real
elapsed time using time.monotonic() (capture start = time.monotonic() before the
loop and compute elapsed = time.monotonic() - start each iteration) or update
elapsed by the actual durations returned from wait_for_response and _stop_event,
then use that real elapsed to compare against max_wait so the timeout reflects
real wall-clock time (refer to wait_for_intent_service, self._stop_event,
self.bus.wait_for_response, max_wait, elapsed).
In `@test/unittests/test_converse_service.py`:
- Around line 53-160: Tests use time.sleep and a timed t.join to wait for
ack_handler and thread completion, causing flakes; replace that with a
threading.Event: in capture_on call event.set() when installing ack_handler,
then in the test wait on event.wait(timeout) before calling ack_handler, and
after invoking ack_handler use t.join() (no timeout) and assert not t.is_alive()
to ensure the worker finished; update all three tests
(test_skill_responds_can_handle_true_is_included,
test_skill_responds_can_handle_false_excluded,
test_malformed_pong_no_skill_id_is_ignored) that call
svc._collect_converse_skills and use ack_handler to synchronize using this Event
instead of time.sleep and timed join.
In `@test/unittests/test_fallback_service.py`:
- Around line 195-265: The tests test_skill_in_range_receives_ping_and_responds
and test_skill_responds_can_handle_false_excluded are flaky due to time.sleep
and join(timeout); replace the sleep-based synchronization with a
threading.Event: create an Event (e.g., handler_ready) that capture_on sets when
it assigns ack_handler, wait handler_ready.wait(timeout) before invoking
ack_handler, and have the worker thread set another Event (e.g., done) or simply
call t.join() (no timeout) after signaling to ensure completion; remove
time.sleep and the join(timeout=1) usage and assert the thread has finished
before reading result_holder so svc._collect_fallback_skills (and ack_handler)
are deterministically synchronized.
In `@test/unittests/test_intent_service_extended.py`:
- Around line 30-53: The test helper _make_service creates an IntentService
without running IntentService.__init__, so the FakeBus never gets the same
registered handlers that shutdown() expects; update _make_service to either call
IntentService.__init__ (with stubs) or explicitly register the same listener
names the real init creates on the FakeBus (the handlers removed in
IntentService.shutdown), and add the missing 'intent.service.intent.get'
listener to the test's assertions so the shutdown harness matches the real
listener set; reference _make_service, IntentService.__init__, shutdown(),
FakeBus and the 'intent.service.intent.get' listener when making the changes.
In `@test/unittests/test_skill_installer.py`:
- Around line 199-210: The current behavior returns True (fail open) on
ConnectionError or non-404 GitHub responses which bypasses validate_skill;
change validate_skill to treat network exceptions and non-404 API errors as
validation failures (return False or raise so pip_install sees a retryable
install failure) instead of True. Update the tests
(test_validate_skill_network_error_fail_open and
test_validate_skill_unexpected_api_error_fail_open) to expect False (or an
appropriate exception) and ensure validate_skill's implementation catches
requests exceptions and non-404 responses and returns False (or re-raises a
specific install error) so pip_install will not accept unvalidated repos.
In `@test/unittests/test_stop_service.py`:
- Around line 169-210: The test currently sends a malformed pong so handle_ack()
returns early; instead simulate the exception path by invoking the ack handler
with a well-formed Message that passes the guard (include skill_id) and force
the handler to raise (e.g., patch or monkeypatch StopService.handle_ack or wrap
ack_handler so it raises when called) so the exception path inside
svc._collect_stop_skills is exercised; ensure you still capture the ack_handler
from svc.bus.on, call it with Message("skill.stop.pong", {"skill_id":
"bad_skill"}) and have the handler raise an Exception, then assert
svc.bus.remove.assert_called_once() as before.
- Around line 510-519: The test test_returns_skill_ids_in_order only asserts
membership but must assert ordering per documented contract: after activating
"skill_b" then "skill_a" the returned list from
StopService.get_active_skills(Message("test")) should have "skill_a" before
"skill_b". Update the assertions to check the exact order (e.g., compare result
to the expected ordered list or assert result.index("skill_a") <
result.index("skill_b")) by locating the test_returns_skill_ids_in_order
function, the Session.activate_skill calls, and the
StopService.get_active_skills invocation.
---
Outside diff comments:
In `@ovos_core/intent_services/stop_service.py`:
- Around line 244-251: The current flow lets a fuzzy global_stop detected by
voc_match(...) fall through to match_low(...) which only scores against stop.voc
and biases stop:skill when active skills exist, losing the global_stop
semantics; change the logic in the match_medium/match handler (the block using
voc_match, get_active_skills, and calling match_low) so that when is_global_stop
is true you either (A) short-circuit and return an explicit global-stop intent
(e.g., the same structure match_low would return for stop:global) or (B) call
match_low with an additional flag/param that forces matching against the
global_stop vocabulary or forces preference for stop:global over stop:skill;
update match_low signature/behavior accordingly so fuzzy global_stop phrases
always map to stop:global, not to stop:skill or fall-through.
---
Duplicate comments:
In `@ovos_core/intent_services/service.py`:
- Around line 321-326: The telemetry upload is gated using
self.config.get("open_data", {}) but _upload_match_data() reads
Configuration().get("open_data", {}), causing a mismatch; fix by making the
gating and uploader use the same config source—either modify _upload_match_data
to read from self.config (preferred) or pass the resolved open_data into
create_daemon when calling _upload_match_data (e.g., call
create_daemon(self._upload_match_data, (open_data, match.utterance, ...))), and
update the _upload_match_data signature accordingly so both checks reference the
same config.
In `@ovos_core/skill_manager.py`:
- Around line 600-613: The code currently holds self._plugin_skills_lock while
calling skill_loader.instance.shutdown() and default_shutdown(), risking
deadlocks; change the flow in unload logic so you acquire the lock only to check
and remove the SkillLoader from self.plugin_skills (e.g. get and pop skill_id
into a local variable), then release the lock and call
skill_loader.instance.shutdown() and skill_loader.instance.default_shutdown()
outside the lock, preserving the same try/except logging behavior around those
calls; keep references to _plugin_skills_lock, plugin_skills, skill_loader,
shutdown and default_shutdown to locate and modify the block.
---
Nitpick comments:
In `@ovos_core/intent_services/locale/ca-es/global_stop.voc`:
- Around line 6-17: The list in global_stop.voc contains duplicate stop phrases
and inconsistent optional-suffix patterns; deduplicate entries by keeping one
canonical form per phrase (e.g., keep a single "acaba-ho tot" and a single
"cessa tot") and normalize the optional "-ho" pattern across all lines (pick one
consistent pattern instead of mixing "(-ho|)" and "(|-ho)"); update the block so
each unique stop phrase appears only once and all optional-suffix variants use
the same canonical regex form.
In `@ovos_core/intent_services/locale/ca-es/stop.voc`:
- Around line 13-14: Normalize the capitalization and punctuation in stop.voc:
change the alternation token `(Atura|para)` to match the project's chosen casing
(e.g., `(atura|para)` or `(Atura|Para)`) so it is consistent with other
alternations, and add the missing comma after the polite phrase "Si us plau" so
the line reads "Si us plau, (atura|para) la tasca actual" (adjust casing to the
consistent style you choose).
In `@ovos_core/intent_services/locale/da-dk/global_stop.voc`:
- Around line 1-16: The vocabulary contains exact duplicate utterances (e.g.,
"stop alt" and "afslutte alt") which should be deduplicated; edit the
global_stop vocabulary block to remove repeated lines so each utterance appears
only once, preserving one instance of each unique Danish phrase and keeping the
file as a newline-separated list of unique utterances (check occurrences of
"stop alt", "afslutte alt", "annuller alle" etc. and remove duplicates).
In `@ovos_core/intent_services/locale/da-dk/stop.voc`:
- Around line 15-16: Remove the duplicate phrase entry "Stop den aktuelle
handling" from the da-dk stop.voc vocabulary file so the line appears only once;
locate the two identical lines and delete one, ensuring the file contains unique
utterances only.
In `@ovos_core/intent_services/locale/de-de/global_stop.voc`:
- Around line 4-5: The pattern "(breche|brech) alles ab" is redundant because
"(breche|brech) (alles) ab" already matches both forms; remove the explicit
"(breche|brech) alles ab" entry from global_stop.voc (or keep it but add a
comment explaining it's for readability) so only the singular pattern
"(breche|brech) (alles) ab" remains to avoid duplicate matches.
In `@ovos_core/intent_services/locale/eu/global_stop.voc`:
- Around line 28-31: Remove the duplicate vocabulary lines
introduced—specifically drop the entries "gelditu dena", "bukatu dena", and
"bukatu gauza guztiak" from the block that also contains "gelditu gauza guztiak"
so only the original single occurrences remain; edit the file's vocabulary list
in ovos_core/intent_services/locale/eu/global_stop.voc to delete those duplicate
literal lines and leave the earlier unique entries intact.
In `@ovos_core/intent_services/locale/fa-ir/stop.voc`:
- Around line 15-16: The file contains a duplicated vocabulary entry "فعالیت
فعلی رو بیخیال شو" in the stop.voc lexicon; remove the redundant occurrence so
that only one instance of "فعالیت فعلی رو بیخیال شو" remains (keep the other
unique entry "فعالیت فعلی رو متوقف کن"), ensuring the stop.voc vocabulary has no
duplicate phrases.
In `@ovos_core/intent_services/locale/gl-es/global_stop.voc`:
- Around line 8-16: The block contains exact duplicate stop utterances; remove
the repeated lines so each phrase appears only once (specifically deduplicate
"parar todo", "paralo todo", and "detelo todo" so only a single occurrence of
each remains), ensure no extra leading/trailing whitespace or blank lines, and
save the updated global_stop.voc with the remaining unique stop variants intact.
In `@ovos_core/intent_services/locale/gl-es/stop.voc`:
- Around line 9-15: Remove the duplicated vocabulary entry "Parar o proceso en
curso" in the stop.voc list so it appears only once; locate both occurrences of
the exact phrase and delete the redundant line, leaving the rest of the entries
and formatting intact.
In `@ovos_core/intent_services/locale/it-it/global_stop.voc`:
- Around line 29-31: Remove the duplicate generated phrase by deleting or
modifying the pattern "(Interrompi|termina|stoppa|ferma) tutte le
(azioni|attività) in (esecuzione|corso)" so it no longer expands to the same
utterance as the existing "(Interrompi|termina|stoppa|ferma) tutte le attività
in esecuzione"; update the remaining patterns in global_stop.voc to ensure each
line produces unique phrases and avoid redundant training/matching weight on
identical sentences.
In `@ovos_core/intent_services/locale/nl-nl/global_stop.voc`:
- Around line 1-13: The NL-NL global stop vocabulary contains repeated lines
(notably "stop alles" and other duplicates); edit global_stop.voc to deduplicate
entries so each utterance appears only once (preserve one occurrence of "stop
alles", "beëindig alles", "alles stoppen", "alles annuleren", "alles afmaken",
"alles afbreken", "alles beëindigen", etc.), resulting in a list of unique stop
phrases to keep the vocabulary clean and maintainable.
In `@ovos_core/intent_services/locale/pl-pl/stop.voc`:
- Around line 8-9: Remove duplicate Polish entries so each phrase appears only
once: locate all occurrences of "Zatrzymaj bieżące działanie" and "Zakończ
bieżące działanie" and keep a single instance of each (remove the extras), and
likewise deduplicate any repeated variants such as "Zatrzymaj bieżący proces" so
the vocabulary file contains unique lines only; ensure quoting/spacing matches
existing entries so intent matching is unchanged.
In `@ovos_core/intent_services/locale/uk-ua/global_stop.voc`:
- Around line 6-15: Add parallel Ukrainian variants using "усі" alongside
existing "всі" entries in the global_stop.voc vocabulary to improve ASR and
speaker-variant matching; update the entries currently containing "всі" (e.g.,
"припиніть всі процеси", "припиніть всі операції", "скасуйте всі завдання",
"завершіть всі дії", etc.) by adding corresponding lines with "усі" (e.g.,
"припиніть усі процеси", "припиніть усі операції", "скасуйте усі завдання",
"завершіть усі дії", etc.) so each original phrase has a direct "усі"
counterpart.
In `@test/end2end/test_intent_pipeline.py`:
- Around line 99-107: The tests currently assert exact external-skill internals
by expecting Message events with data name
"CountSkill.handle_how_are_you_intent" which couples the routing tests to
ovos-skill-count implementation; change those assertions to check the generic
routing/event identity instead (e.g., assert the "mycroft.skill.handler.start"
and "mycroft.skill.handler.complete" messages are emitted and verify a
non-implementation-specific identifier such as the intent name or skill_id in
data/context rather than the Python method string), updating the Message checks
in test_intent_pipeline.py (the blocks referencing
"mycroft.skill.handler.start"/"mycroft.skill.handler.complete" and
"CountSkill.handle_how_are_you_intent") to validate routing without the exact
method name.
In `@test/end2end/test_stop_refactor.py`:
- Around line 196-205: The test currently uses time.sleep(2) after
create_daemon(make_it_count), which is flaky; instead register a temporary
listener on self.minicroft.bus that waits for the concrete bus event emitted by
the skill when the background "count to infinity" handler reaches running (e.g.,
the skill-specific running/start event), use a threading.Event (or similar) that
the listener sets, call create_daemon(make_it_count) then wait on that event
with a short timeout, and only proceed to send the stop utterance once the event
is set; reference make_it_count, create_daemon, self.minicroft.bus.emit, session
and self.skill_id when locating where to add the listener and event handling.
In `@test/unittests/test_skill_installer.py`:
- Around line 293-299: Add a new assertion block to cover the successful
uninstall path in test_handle_uninstall_skill: call
skills_store.handle_uninstall_skill with a Message containing a valid skill id
(ensuring the id will be normalized), mock skills_store.pip_uninstall to return
success, assert pip_uninstall was called with the normalized skill id, assert no
play_error_sound was triggered, and assert the bus emitted the success message
type (e.g., "ovos.skills.uninstall.complete") with appropriate success data
rather than the failure/error payload.
In `@test/unittests/test_transformers.py`:
- Around line 248-336: Add tests mirroring the UtteranceTransformersService
cache-clear check to cover the other services: set a sentinel on the service
instance's _sorted_plugins, call MetadataTransformersService.load_plugins and
IntentTransformersService.load_plugins, and assert the sentinel is cleared; use
the same helper mocks/patches as existing tests (patch
find_metadata_transformer_plugins / find_intent_transformer_plugins and
Configuration) and add two new test methods (e.g.
test_metadata_load_plugins_clears_sorted_cache and
test_intent_load_plugins_clears_sorted_cache) in test_transformers.py that set
svc._sorted_plugins to a non-None value, invoke load_plugins(), and assert
svc._sorted_plugins is reset.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 97ea9b2f-c4a6-464b-9d83-dc6fd6a6ba6b
📒 Files selected for processing (49)
FAQ.mdMAINTENANCE_REPORT.mdSUGGESTIONS.mdovos_core/intent_services/converse_service.pyovos_core/intent_services/locale/ca-es/global_stop.vocovos_core/intent_services/locale/ca-es/stop.vocovos_core/intent_services/locale/da-dk/global_stop.vocovos_core/intent_services/locale/da-dk/stop.vocovos_core/intent_services/locale/de-de/global_stop.vocovos_core/intent_services/locale/de-de/stop.vocovos_core/intent_services/locale/en-us/global_stop.vocovos_core/intent_services/locale/en-us/stop.vocovos_core/intent_services/locale/es-es/global_stop.vocovos_core/intent_services/locale/es-es/stop.vocovos_core/intent_services/locale/eu/global_stop.vocovos_core/intent_services/locale/eu/stop.vocovos_core/intent_services/locale/fa-ir/global_stop.vocovos_core/intent_services/locale/fa-ir/stop.vocovos_core/intent_services/locale/fr-fr/global_stop.vocovos_core/intent_services/locale/fr-fr/stop.vocovos_core/intent_services/locale/gl-es/global_stop.vocovos_core/intent_services/locale/gl-es/stop.vocovos_core/intent_services/locale/it-it/global_stop.vocovos_core/intent_services/locale/it-it/stop.vocovos_core/intent_services/locale/nl-be/stop.vocovos_core/intent_services/locale/nl-nl/global_stop.vocovos_core/intent_services/locale/nl-nl/stop.vocovos_core/intent_services/locale/pl-pl/global_stop.vocovos_core/intent_services/locale/pl-pl/stop.vocovos_core/intent_services/locale/pt-br/global_stop.vocovos_core/intent_services/locale/pt-br/stop.vocovos_core/intent_services/locale/pt-pt/global_stop.vocovos_core/intent_services/locale/pt-pt/stop.vocovos_core/intent_services/locale/uk-ua/global_stop.vocovos_core/intent_services/locale/uk-ua/stop.vocovos_core/intent_services/service.pyovos_core/intent_services/stop_service.pyovos_core/skill_installer.pyovos_core/skill_manager.pypyproject.tomltest/end2end/test_intent_pipeline.pytest/end2end/test_stop.pytest/end2end/test_stop_refactor.pytest/unittests/test_converse_service.pytest/unittests/test_fallback_service.pytest/unittests/test_intent_service_extended.pytest/unittests/test_skill_installer.pytest/unittests/test_stop_service.pytest/unittests/test_transformers.py
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
ovos_core/skill_installer.py (1)
336-342:⚠️ Potential issue | 🟠 MajorUncaught exception from
pip_uninstallprevents failure reply.
pip_uninstall()raisesRuntimeErroron subprocess failures (line 219). This exception escapes the handler, so clients never receiveovos.skills.uninstall.failed. This was flagged in a previous review and remains unaddressed.🐛 Proposed fix to catch exceptions
# Treat skill_id as a package name (e.g., 'skill-name.author' -> 'skill-name-author') # or accept directly as package name pkg_name = skill.replace(".", "-") if "." in skill else skill - if self.pip_uninstall([pkg_name]): + try: + success = self.pip_uninstall([pkg_name]) + except Exception: + LOG.exception(f"Failed to uninstall skill: {skill}") + self.bus.emit(message.reply("ovos.skills.uninstall.failed", + {"error": InstallError.PIP_ERROR.value})) + return + + if success: LOG.info(f"Successfully uninstalled skill: {skill}") self.bus.emit(message.reply("ovos.skills.uninstall.complete")) else:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovos_core/skill_installer.py` around lines 336 - 342, Wrap the call to pip_uninstall in a try/except so RuntimeError from pip_uninstall doesn't escape; in the uninstall handler (the method containing the shown snippet) catch RuntimeError (or Exception) around self.pip_uninstall([pkg_name]), log the exception via LOG.exception or LOG.error including the error details, and in the except block emit the failure reply using self.bus.emit(message.reply("ovos.skills.uninstall.failed", {"error": InstallError.PIP_ERROR.value})); keep the existing success branch unchanged.
🧹 Nitpick comments (2)
test/unittests/test_skill_installer.py (2)
9-9: Use explicitNoneunion for optional parameter.PEP 484 prohibits implicit
Optional. The type hint should explicitly indicateNoneis allowed.♻️ Proposed fix
-def _make_github_response(status_code: int = 200, file_names: list = None, +def _make_github_response(status_code: int = 200, file_names: list | None = None, ok: bool = True) -> MagicMock:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_skill_installer.py` at line 9, The helper function _make_github_response currently types file_names as "list" with a default of None; update its signature to explicitly allow None (e.g., use Optional[List[str]] or list[str] | None) and add the required typing imports (Optional and List if using typing) so the parameter type follows PEP 484 and clearly indicates the optional nature of file_names.
267-271: Error message says "install" in an uninstall context.The test asserts
"no packages to install"for an uninstall failure, which is confusing. TheInstallError.NO_PKGSenum is reused for both install and uninstall, but its value is install-specific. Consider whether a separate error value or message would improve clarity.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_skill_installer.py` around lines 267 - 271, The test asserts an uninstall failure but the error text uses an install-specific message; update the handling to produce an uninstall-appropriate message: either change the InstallError enum to add a distinct value (e.g., InstallError.NO_PKGS_UNINSTALL) or adjust handle_uninstall_skill to map InstallError.NO_PKGS to "no packages to uninstall" (or otherwise translate the enum) so skills_store.handle_uninstall_skill returns/dispatches "no packages to uninstall" instead of "no packages to install"; reference InstallError.NO_PKGS and the handle_uninstall_skill function to locate the fix.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ovos_core/skill_installer.py`:
- Around line 229-250: The docstring for the GitHub validation routine in
ovos_core/skill_installer.py is inaccurate: it claims the function scans
pyproject.toml/setup.cfg for legacy class names but the implementation only
checks file presence. Either update the docstring to remove/clarify the
legacy-class scanning claim, or implement the scanning behavior in the
validation function (validate_github_repo) by fetching the contents of
pyproject.toml, setup.cfg or setup.py via the GitHub contents API (respecting
the existing 3s timeout/fallback behavior) and searching for the identifiers
"MycroftSkill" and "CommonPlaySkill", returning False if found; keep existing
repo-existence and packaging-file presence checks intact.
In `@test/unittests/test_skill_installer.py`:
- Around line 185-207: The tests test_validate_skill_setup_cfg_valid and
test_validate_skill_dot_git_suffix_stripped mock two HTTP responses but the
current validate_skill implementation only performs the GitHub contents request,
so remove the unused second mock response from mock_get.side_effect in both
tests (delete the _make_manifest_response(...) entries) and adjust assertions if
needed; locate these changes in the test functions
test_validate_skill_setup_cfg_valid and
test_validate_skill_dot_git_suffix_stripped and update only the
mock_get.side_effect arrays so they match the actual single requests made by
validate_skill in ovos_core.skill_installer.
- Around line 155-162: The test test_validate_skill_valid_ovos_skill currently
supplies two mocked responses for requests.get but validate_skill no longer
fetches file contents a second time; update the test to match the current
validate_skill behavior by removing the unused manifest mock and only mocking
the single repository contents API call (adjust mock_get.side_effect to a single
response produced by _make_github_response with
file_names=["pyproject.toml","README.md"]) so the test exercises validate_skill
as implemented; keep the existing assert that
skills_store.validate_skill("https://github.com/openvoiceos/skill-foo") is True
and reference the test function name and the validate_skill method when making
the change.
---
Duplicate comments:
In `@ovos_core/skill_installer.py`:
- Around line 336-342: Wrap the call to pip_uninstall in a try/except so
RuntimeError from pip_uninstall doesn't escape; in the uninstall handler (the
method containing the shown snippet) catch RuntimeError (or Exception) around
self.pip_uninstall([pkg_name]), log the exception via LOG.exception or LOG.error
including the error details, and in the except block emit the failure reply
using self.bus.emit(message.reply("ovos.skills.uninstall.failed", {"error":
InstallError.PIP_ERROR.value})); keep the existing success branch unchanged.
---
Nitpick comments:
In `@test/unittests/test_skill_installer.py`:
- Line 9: The helper function _make_github_response currently types file_names
as "list" with a default of None; update its signature to explicitly allow None
(e.g., use Optional[List[str]] or list[str] | None) and add the required typing
imports (Optional and List if using typing) so the parameter type follows PEP
484 and clearly indicates the optional nature of file_names.
- Around line 267-271: The test asserts an uninstall failure but the error text
uses an install-specific message; update the handling to produce an
uninstall-appropriate message: either change the InstallError enum to add a
distinct value (e.g., InstallError.NO_PKGS_UNINSTALL) or adjust
handle_uninstall_skill to map InstallError.NO_PKGS to "no packages to uninstall"
(or otherwise translate the enum) so skills_store.handle_uninstall_skill
returns/dispatches "no packages to uninstall" instead of "no packages to
install"; reference InstallError.NO_PKGS and the handle_uninstall_skill function
to locate the fix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: be0f0b98-6148-47eb-9bd8-5b6436baefaf
📒 Files selected for processing (8)
.github/workflows/coverage.yml.github/workflows/ovoscope.ymlFAQ.mdMAINTENANCE_REPORT.mdQUICK_FACTS.mdovos_core/skill_installer.pypyproject.tomltest/unittests/test_skill_installer.py
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/docs_check.yml:
- Around line 10-11: Replace the mutable branch reference in the reusable
workflow invocation by pinning the `uses:
OpenVoiceOS/gh-automations/.github/workflows/docs-check.yml@dev` to a full
commit SHA (e.g., change `@dev` to `@<commit-sha>`) and keep `secrets: inherit`
only if you trust that fixed SHA; update the same pattern in other workflows
that reference OpenVoiceOS/gh-automations (type_check.yml, repo_health.yml,
sync_translations.yml, release_workflow.yml, release_preview.yml,
publish_stable.yml, pipaudit.yml, ovoscope.yml, locale_check.yml) so all `uses:`
lines point to immutable commit SHAs instead of `@dev`.
In @.github/workflows/locale_check.yml:
- Around line 10-11: Replace the mutable ref "@dev" used in the reusable
workflow call with an immutable commit SHA: update the uses line (uses:
OpenVoiceOS/gh-automations/.github/workflows/locale-check.yml@dev) to reference
the full commit SHA of the target repo, keep secrets: inherit as-is, and verify
the chosen SHA corresponds to the intended workflow commit in
OpenVoiceOS/gh-automations to ensure the workflow is pinned immutably.
In @.github/workflows/repo_health.yml:
- Around line 10-11: Replace the mutable branch ref in the reusable workflow
invocation by pinning the `uses:
OpenVoiceOS/gh-automations/.github/workflows/repo-health.yml@dev` entry to the
specific commit SHA of the external workflow, and remove `secrets:
inherit`—instead explicitly pass only the required secrets (e.g. build/repo
tokens needed by this workflow) by name; update the `uses:` line to the commit
SHA and add a `secrets:` mapping that lists each required secret rather than
inheriting all secrets.
In @.github/workflows/sync_translations.yml:
- Around line 13-14: The workflow currently references a mutable branch in the
reusable workflow via the line starting with "uses:
OpenVoiceOS/gh-automations/.github/workflows/sync-translations.yml@dev" which is
unsafe when combined with "secrets: inherit"; replace the branch ref "@dev" with
the corresponding immutable full commit SHA for that reusable workflow so the
"uses" entry pins to a specific commit, keeping the "secrets: inherit" behavior
unchanged.
In @.github/workflows/type_check.yml:
- Around line 10-11: Replace the mutable branch reference in the reusable
workflow invocation "uses:
OpenVoiceOS/gh-automations/.github/workflows/type-check.yml@dev" with an
immutable commit SHA (e.g. @<commit-sha>) so the workflow is pinned; update the
line that currently reads "uses:
OpenVoiceOS/gh-automations/.github/workflows/type-check.yml@dev" to "uses:
OpenVoiceOS/gh-automations/.github/workflows/type-check.yml@<commit-sha>" and
keep "secrets: inherit" as-is; apply the same SHA-pinning pattern to the other
occurrences of the same reusable workflow references across the repo.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 52a7ec4b-a4d1-4938-9049-24b03a047fe8
📒 Files selected for processing (6)
.github/workflows/docs_check.yml.github/workflows/locale_check.yml.github/workflows/release_preview.yml.github/workflows/repo_health.yml.github/workflows/sync_translations.yml.github/workflows/type_check.yml
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/release_preview.yml
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Performance Improvements
Documentation
Localization
Tests / CI