Release 9.3.12a1 - #533
Open
github-actions[bot] wants to merge 178 commits into
Open
Conversation
fix: standalone skills wait_for_core
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
* Add French workshop locale resources * Refine French workshop dialogs * Polish French workshop voice prompts
* fix: standalone skills wait_for_core
* fix: standalone skills wait_for_core
* fix: standalone skills wait_for_core
* fix: repair 6 failing tests and raise coverage from 45% to 54%
Test fixes:
- test_decorators.py: update TestKillableIntents for renamed class
(KillableSkill → TestAbortSkill) and language-agnostic speak assertions
- test_decorators.py: fix test_get_response — abort_question must carry
matching session_id; the killable_event session check silently ignores
messages from a different session (root cause of the flaky failure)
- test_decorators.py: fix activate_msg type (intent.service.skills.activate
→ skill.converse.get_response.enable) per current ovos-workshop behaviour
- test_abstract_app.py: fix test_gui_init — GUIInterface.__len__==0 makes
an empty gui falsy, so OVOSSkill._startup creates a new SkillGUI; assert
type rather than identity
- skills/test_base.py: fix test_default_shutdown — change `if self.gui:` to
`if self.gui is not None:` in OVOSSkill.default_shutdown so shutdown is
always called on the SkillGUI regardless of its data contents
New tests (12 files, 170 new test cases):
- test_version.py, test_settings.py, test_permissions_extended.py
- test_backwards_compat.py, test_intents_extended.py
- test_decorators_layers_extended.py, test_resource_files_extended.py
- test_simple_imports.py (passive, layers, fallback_handler)
- skills/test_converse_extended.py, skills/test_auto_translatable_extended.py
- skills/test_common_play_extended.py, skills/test_game_skill_extended.py
- skills/test_intent_provider.py
Documentation (new + improved):
- docs/app.md, docs/game-skill.md, docs/auto-translatable.md
- docs/skill-api.md, docs/filesystem.md
- Improved docs/decorators.md (line citations, abort flow, OCP section)
- Improved docs/index.md (full class hierarchy, navigation table)
- Improved docs/skill-classes.md (added game, universal, active classes)
- FAQ.md, QUICK_FACTS.md, MAINTENANCE_REPORT.md, AUDIT.md, SUGGESTIONS.md
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* fix: audit — critical signature bug, bare excepts, missing type annotations
CRITICAL fix (common_play.py:466,481,492,501):
__handle_ocp_pause/resume/next/prev each inspected signature(self.__playback_handler)
to decide whether to pass `message=` to the OTHER handler. Replace with the
correct handler's own signature in each case.
Bare except → explicit (5 locations):
ovos.py:966 _cq_handler call
ovos.py:1216 intent_file.build() — now logs warning
ovos.py:1450 intent_parser.build() — now logs warning
common_query_skill.py:177 CQS_match_query_phrase
resource_files.py:89 language-tag distance computation
Type annotations added (ovos.py):
load_regex_files, find_resource, _handle_first_run, _check_for_first_run,
on_ready/error/stopping/alive/started_status, _handle_settings_changed,
__handle_get_response, _handle_killed_wait_response
Type annotations added (fallback.py):
register_fallback: callable -> Callable, added -> None, added Callable import
AUDIT.md updated with all findings (fixed + open), including:
- race condition on __responses dict
- busy-wait sleep loops in get_response
- god class note (OVOSSkill 2500 lines)
- magic numbers and variable shadowing
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* build: migrate to pyproject.toml, add ovos-gui-api-client dependency
- Add pyproject.toml (setuptools backend) replacing setup.py
- Delete setup.py and MANIFEST.in (superseded by pyproject.toml)
- Add ovos-gui-api-client>=0.1.0,<1.0.0 to requirements.txt
- Update app.py import: ovos_bus_client.apis.gui → ovos_gui_api_client
- Add __version__ string to version.py derived from VERSION_* constants
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* deprecate: IdleDisplaySkill and resting_screen_handler decorator
The homescreen / idle display is now managed by the shell and its adapter
plugin (ovos-shell). Skills must no longer subclass IdleDisplaySkill or
register with the homescreen manager directly.
Changes:
- idle_display_skill.py: replace full implementation with a deprecation stub
that logs a warning on instantiation; retains the class name for import
compatibility
- skills/__init__.py: remove IdleDisplaySkill from the public API exports
- decorators/__init__.py: remove resting_screen_handler decorator (replaced
by the @homescreen_app decorator on OVOSSkill)
- test_idle_display_skill.py: update test to only assert the stub exists
- test_skill.py: remove broken test_stop test (tracked in AUDIT.md — Mock
is not JSON-serialisable through FakeBus)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* ci: migrate workflows to OpenVoiceOS/gh-automations@dev
- coverage.yml: update actions/checkout and actions/setup-python to v4/v5
- downstream.yml: replace bespoke pipdeptree script with reusable
gh-automations downstream-check.yml workflow
- license_tests.yml: switch from neongeckocom to gh-automations
license-check.yml; add dev branch PR trigger
- publish_stable.yml: migrate from TigreGotico gh-automations to
OpenVoiceOS gh-automations; remove inline build/publish steps now
handled by the reusable workflow; add sync_dev and publish_pypi flags
- release_workflow.yml: migrate from TigreGotico; collapse ~90 lines of
inline steps into the reusable publish-alpha.yml call; add
workflow_dispatch trigger; enable propose_release and notify_matrix
- unit_tests.yml: delete (replaced by build_tests.yml which uses the
standardised gh-automations build-tests.yml reusable workflow)
- build_tests.yml: new workflow using gh-automations build-tests.yml
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* fix: revert accidental GUI client refactor and pyproject.toml migration
* update
* update
* update
* update
* update
* fix: catch AbortEvent in killable thread to prevent unhandled thread exception
pytest ≥3.11 ships a threadexception plugin that treats any unhandled
exception raised from a daemon thread as a test failure. When
killable_event kills a running intent thread via raise_exc(AbortEvent),
the exception propagates up through the handler and exits the thread
without being caught, causing non-deterministic pytest failures.
Wrap the decorated function in a _guarded closure that catches AbortEvent
so the thread exits cleanly — it is an intentional kill, not an error.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* fix: narrow except clause and improve error logging
- resource_files.py: narrow bare `except Exception` to `except ValueError`
in locate_lang_directories; tag_distance only raises ValueError for invalid
language codes — catching everything masks unrelated errors
- skills/ovos.py: include exception instance in initialization failure log
so the root cause is visible in traces
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* test: address CodeRabbit review comments in unit tests
- test_abstract_app.py: fix constructor typo __int__ → __init__ (critical;
subclass constructor was never called)
- test_idle_display_skill.py: replace misleading hasattr check with
assertion that handle_idle is abstract
- test_converse_extended.py: tighten assertIsInstance(dict) to assertEqual({})
for converse_matchers initialization
- test_common_play_extended.py: save/restore XDG env vars in tearDown;
tighten ocp_cache_dir assertion using os.path.basename; add initial_count+1
assertion to test_register_media_type
- test_decorators_layers_extended.py: add assertFalse(is_active) assertion
after activate_layer on non-existent layer
- test_intent_provider.py: add actual DeprecationWarning assertion using
warnings.catch_warnings (removes module from cache first so warning fires)
- test_decorators.py: remove unnecessary f-prefix from string literals with
no interpolation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* ci: fix existing workflows and add missing standard gh-automations workflows
Fixes:
- license_tests.yml: remove invalid empty with: block; add secrets: inherit
- build_tests.yml: add dev to push trigger branches (post-merge CI now runs)
Add missing standard reusable workflows (all using @dev ref):
- test.yml: unit tests on every PR/push via build-tests.yml
- lint.yml: ruff linting via lint.yml
- pip_audit.yml: CVE dependency scan via pip-audit.yml
- repo_health.yml: repo hygiene checks via repo-health.yml
All workflows reference OpenVoiceOS/gh-automations@dev per workspace policy.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* docs: fix CodeRabbit review comments in docs and FAQ
- docs/game-skill.md: add 'text' language specifier to fenced code block
- FAQ.md: fix path examples to use '.' instead of 'ovos-workshop/' prefix
- MAINTENANCE_REPORT.md: create required maintenance log (first entry)
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
---------
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Rename all locale directories to use canonical BCP-47 codes: - Lowercase regions → uppercase (da-dk → da-DK, en-us → en-US) - Bare language codes → full BCP-47 (da → da-DK, eu → eu-ES) - Invalid codes remapped (eu-eu → eu-ES, fa-fa → fa-IR) - Nested bare-language subdirs merged up (eu-ES/eu/ → eu-ES/) This fixes wheel build failures caused by ZIP files containing duplicate entries with different contents for the same logical path. Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
#514) _wait_response() busy-polled `while not ans: time.sleep(0.1)` with no deadline while waiting on the killable background thread (_real_wait_response) to populate __validated_responses. If that thread never finishes (eg. the bus/TTS handshake it waits on inside __get_response never completes), the caller blocks indefinitely instead of returning the documented "no response" (None) outcome. Add a deadline derived from the same budget the background thread itself uses per attempt: skills.get_response_timeout (default 20s) plus the 15s wait_while_speaking() ceiling already used elsewhere in this file for a reprompt, multiplied by the number of retry attempts. Normal-path and abort-path (externally set None) behavior is unchanged; only the previously-infinite stall case now returns None within a bounded time. Field evidence: OpenVoiceOS/ovos-skill-alerts#138 "Update 3" captured a live py-spy stack of ask_yesno -> get_response -> _wait_response blocked forever in exactly this loop, hanging a CI job to its 30-minute kill.
…eak on natural completion (#513) create_killable_daemon already marks the wrapper thread as daemon, but that guarantee lives entirely in the ovos-utils dependency; enforce t.daemon = True in killable.py itself too, so a leaked get_response()/ killable_intent waiter thread can never block process exit regardless of what ovos-utils does. More importantly, every call to a killable_intent/killable_event wrapped method registers two `.once` bus listeners (abort msg + skill stop msg) and appends the thread to skill._threads, but neither was ever cleaned up when the wrapped function finished on its own (no abort message ever arrived). Over the life of a long-lived skill instance - eg. a test suite sharing one skill across many calls, or an embedded/CI process - this accumulates one stale listener/thread reference per call, which is exactly the pattern surfaced by CI thread dumps in ovos-skill-alerts#123 (killable.py / get_response `_guarded` threads and their listeners piling up as more tests run against the same shared skill). _guarded now unregisters both listeners and drops the thread from skill._threads in a finally block, so cleanup happens whether the thread is killed or finishes normally. Fixes: OpenVoiceOS/ovos-skill-alerts#138
disable_intent()/enable_intent() never actually restored a handler: - OVOSSkill.enable_intent() called register_intent_file(name, None) / register_intent(intent, None); both register_* paths skip add_event() whenever handler is falsy, so nothing was ever rebound. - IntentServiceInterface.remove_intent() was called with the skill_id-prefixed name, but registered_intents/detached_intents are keyed by the bare name (register_intent/register_template strip the prefix before storing), so the detach bookkeeping never matched and disable_intent's internal state was already wrong going in. - register_template() also never dropped a re-registered intent from detached_intents (register_intent already did), so intent_is_detached() kept reporting True forever after a padatious intent was re-enabled. Fix: OVOSSkill now remembers each intent's handler in self._intent_handlers at registration time and passes it back into register_intent_file/register_intent on enable_intent(), instead of None. remove_intent() normalizes the skill_id prefix before matching against the bare-keyed registry, and register_template() clears the matching detached_intents entry on re-registration. Reproduced on origin/dev; found during adversarial review of #500.
locate_lang_directories and _get_closest_lang each reimplemented the langcodes distance check with their own inlined '< 10' threshold and a copy of the same explanatory comment. That policy belongs in one place. Delegate both to ovos_spec_tools.lang_matches / closest_lang, which own the OVOS-INTENT-2 §2.2 language-distance policy. Requires ovos-spec-tools>=1.6.0a2: a macrolanguage member scores exactly 10 against its macrolanguage (arz/ar, wuu/zh) and the bound was exclusive before that release, so those pairs resolved to nothing. Without the floor bump this change would silently stop matching them. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
… load (#523) can_answer is decorated with @abc.abstractmethod, but FallbackSkill used the default metaclass, so the declaration was inert. A skill that never implemented can_answer was created without complaint and then raised NotImplementedError inside _handle_fallback_ack, which is registered with speak_errors=False. No pong was emitted, the skills service never routed to the skill, and the user heard nothing. Two official skills shipped in that state. Declaring the metaclass makes the missing method a load error, which is what the abstractmethod already said it was. Only FallbackSkill is changed here. ConversationalSkill, IdleDisplaySkill and OVOSGameSkill declare inert abstract methods too, but enforcing those turns the shipped ActiveSkill and PassiveSkill into abstract classes, so they need a contract decision first.
* fix: keep context-only requires out of INTENT-4 keyword emit
An adapt intent expresses OVOS-CONTEXT-1 gating by requiring an entity
that only intent-context injection can supply (`.require("prev_dialog")`).
That vocabulary has no samples, so `_spec_keyword_descriptors` silently
dropped it from the `ovos.intent.register.keyword` payload. The consumer
then registered a second, ungated copy of the intent under the same name,
and that copy matched any utterance carrying the remaining keywords - with
no context set at all.
Skip the spec emit for a language when any required, one_of or excluded
vocabulary has no samples, and log why. The legacy `register_intent` emit
still carries the complete definition, so gating keeps working.
Regression test fails before this change and passes after.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
* fix: suppress spec keyword emit for context-only optional() keywords too
optional() keywords with no vocab samples (e.g. an OVOS-CONTEXT-1 context
slot) were excluded from the missing-vocab check in
_emit_spec_keyword_intent, so the spec twin still emitted with an empty
"optional" list. The ungated twin then won matches over the legacy parser,
silently dropping the skill's optional context slot from match data.
Fold optional_names into the same missing-vocab check used for
required/excluded/one_of, so a context-only optional() also suppresses the
spec emit and the legacy (context-aware) registration stands alone.
Adds a producer->consumer regression test wiring a real
IntentServiceInterface to a real ovos_adapt.opm.AdaptPipeline: asserts the
consumer ends up holding exactly one parser for the intent, for both the
require() and optionally() context-only cases. Verified this test fails
(2 emits reach the pipeline) against dev's pre-PR-525 intents.py, proving
it catches the original bug class.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
---------
Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…from set_context (#527) OVOSSkill.set_context/remove_context rewrite the context as alphanumeric_skill_id + context (munged, no separator) for the legacy ADAPT dialect. The declarative OVOS-CONTEXT-1 gate resolves a private declaration to <raw_skill_id>:<key> (colon-separated, unsanitized) - the two spellings never coincide, so requires_context/excludes_context gating was unreachable from the real skill API. Keep emitting the munged data['context'] (legacy consumers depend on the exact spelling) and additionally emit data['key'] with the original, unmunged context argument, symmetrically for set_context and remove_context. Round 2 (pair review): _AdaptIntentApi.set_context/remove_context derived the outgoing message's skill_id from whatever ambient Message dig_for_message() found on the call stack, stamping it only if absent. A skill calling set_context while handling a DIFFERENT skill's message (or any receiver of the mycroft.skill.set_cross_context broadcast, whose message is stamped by the originator) had its resolved-key mirror forged under the wrong owner. Fixed by building a forward/copy and stamping out.context["skill_id"] = self.skill_id unconditionally, never mutating the dug ambient message. Also threaded original_key through the deprecated set_adapt_context/remove_adapt_context shortcuts, which apply no munging - context IS the original key there. Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…-tools (#500) * refactor: register canonical intent topics, drop the suffixed twin The dispatch topic is `<skill_id>:<intent_name>` (OVOS-MSG-1 §2.1.1). The `.intent` extension is an authoring resource detail (OVOS-INTENT-2 §3), so it must not reach the wire. `register_intent_file` now derives the name with `canonical_intent_topic` and binds one event, the canonical one. The dual add_event/remove_event on the suffixed twin is gone; `disable_intent` / `enable_intent` / `register_intent_layer` canonicalize the author-supplied name. Old containerized skills still listen on the suffixed topic. That compat now lives in ovos-spec-tools (`intent_topics`), gated by `emit_legacy` at the bus layer, and no longer in the skill layer. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test: pin the intent-registration-normalization alias-collapse Relocated from ovoscope#127 (TestRegistrationNormalizationDedup) at the maintainer's request: ovoscope is the harness library, not the stack, and these guards belong next to the code they pin. A legacy `X:Y.intent`-registered handler must still fire exactly once on a canonical `X:Y` dispatch, and registering both spellings for one intent must collapse to a single fire, not double-dispatch. Uses ovos_spec_tools.intent_topics.IntentAliasRegistry (the real alias-collapse API — the ovoscope draft referenced a nonexistent ovos_spec_tools.intent_compat.normalize_intent_registration). Both pass outright since #500 already registers canonical-only. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * test: collapse legacy intent names with canonical_intent_topic The alias registry is gone from ovos-spec-tools. The two guards assert the same behavior with the pure helper the registry wrapped. * fix: canonical intent names in enable/disable lookups > 🤖 Auto-generated by Claude Sonnet 4.5 via Claude Code — NOT human-reviewed. Verify before acting. register_intent_file() (#500) now binds only the canonical, suffix-less intent topic (OVOS-MSG-1 §2.1.1), but several lookup paths in the enable/disable flow still compared against the author-facing ".intent"-suffixed spelling instead of canonicalizing first, so they never matched: - IntentServiceInterface.intent_is_detached()/get_intent() compared intent_name verbatim against the bare-canonical keys stored in registered_intents/detached_intents; remove_intent() only stripped the "<skill_id>:" prefix, not ".intent". All three now normalize through the existing _clean_padatious_name() helper. - OVOSSkill.handle_enable_intent()/handle_disable_intent() compared the raw bus-payload intent_name against the canonical-keyed detached_intents/registered_intents lists, so a mycroft.skill.enable_intent/disable_intent message carrying the author's ".intent"-suffixed name silently no-op'd. Both now canonicalize via canonical_intent_topic() before comparing. - (found in adversarial re-review) _intent_handlers was still keyed by the RAW spelling passed to register_intent_file() ("time.intent"), while enable_intent() looked the handler up by whatever spelling ITS caller used. That made disable_intent("time") succeed (the registry itself is canonical-keyed) while enable_intent("time") failed with "no handler is on record" -- a one-way door: an intent disabled by its canonical name could never be re-enabled that way, a regression this PR's canonicalization opened (dev fails both spellings symmetrically). Fixed by keying _intent_handlers canonically at register_intent_file()/_register_adapt_intent() (already canonical for the adapt path) and looking it up canonically in enable_intent(); the padatious-vs-adapt re-registration branch now checks the registry entry's type (dict vs IntentBuilder/Intent) instead of the caller's spelling, since a caller may legitimately ask for the canonical name of a ".intent"-authored intent. Also: the killable-intent leak-fix regression test (#513) added handle_test_quick_intent()/"test4.intent" to the shared abort-test fixture skill but never added the locale/en-US/test4.intent resource file, so registration silently skipped it ("Unable to find test4.intent"); and the test itself dispatched on the legacy "skill_id:test4.intent" topic instead of the canonical "skill_id:test4" used by every sibling case in the file. Added the missing fixture and fixed the dispatch topic. test_skill.py::test_intent_register's expected padatious-intent list predates test4 and needed the same addition. test_base.py::test_disable_intent predates the canonical-only refactor (bea570b) and pinned the now-intentionally-removed dual legacy+canonical event binding; updated to assert canonical-only binding, matching test_disable_intent_removes_the_canonical_event. Added test_disable_enable_intent_canonical_spelling_round_trip: proves disable_intent()/enable_intent() work with the canonical spelling (not just the ".intent"-suffixed one) and that re-registration leaves exactly one bus listener on the canonical topic. Fixes 4 CI failures on run 31713513709 (all pythons): test_base.py::{test_disable_intent, test_handle_disable_intent, test_handle_enable_intent}, test_decorators.py::test_no_leak_on_natural_completion, plus the enable/disable one-way-door regression found in adversarial re-review. Full suite: 571 passed (was 566 passed / 4 failed). --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
register_entity_file() named the entity
<skill_id>:<basename>_<md5(entity_file)>. The entity name IS the wire
contract: a consumer resolves a {slot} in a template by the raw slot
token, so no consumer could ever match the hash-suffixed name and every
file-registered entity became an unconstrained wildcard slot.
The hash disambiguated nothing: <skill_id>: already namespaces the
entity, and the hash was taken over the file name that is already part
of the key. The spec-topic emit already stripped it via
_clean_padatious_name, so the two wire contracts disagreed on the name
of one entity.
Emit <skill_id>:<entity> on both contracts.
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
2 participants
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.
Human review requested!