refactor: route OVOSSkill resource loading through ovos-spec-tools (back-compat)#413
Closed
JarbasAl wants to merge 34 commits into
Closed
refactor: route OVOSSkill resource loading through ovos-spec-tools (back-compat)#413JarbasAl wants to merge 34 commits into
JarbasAl wants to merge 34 commits into
Conversation
…ec-tools Wave 2 of the OVOS migration (OpenVoiceOS/architecture#7). Replace reimplemented language-matching and template-expansion logic with the conformant reference implementation from ovos-spec-tools. - resource_files.py: locate_lang_directories now uses ovos_spec_tools.lang_distance instead of langcodes.tag_distance; vocabulary/intent loading uses ovos_spec_tools.expand instead of ovos_utils.bracket_expansion.expand_template. expand is single-spaced per OVOS-INTENT-1, fixing double spaces left when an optional segment is dropped. sorted() preserves the previous deterministic ordering. - converse.py: _get_closest_lang uses ovos_spec_tools.closest_lang, which standardizes both tags and applies the spec's distance-below-10 threshold internally. - Add ovos-spec-tools>=0.0.1a2 to dependencies. All migrated symbols were internal-only; no public API changes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Wave 2 of the OVOS migration (OpenVoiceOS/architecture#7). Replace the reimplemented MustacheDialogRenderer-based dialog rendering with the conformant reference renderer from ovos-spec-tools. - resource_files.py: DialogFile.render / IntentFile.render now load the raw phrase list and render it with ovos_spec_tools.render, which expands (a|b)/[x] variants and fills {name} slots per OVOS-DIALOG-1. SkillResources.render_dialog renders directly without routing through a MustacheDialogRenderer. SkillResources keeps handling the legacy resource types (qml, json, list, value, regex, word, template) that ovos-spec-tools does not cover. - ovos.py: OVOSSkill gains render_dialog, which renders via self.resources.render_dialog; speak_dialog and the get_response / ask on-fail flows call render_dialog instead of dialog_renderer.render. - The SkillResources.dialog_renderer and OVOSSkill.dialog_renderer properties are kept as deprecated compat shims (warnings.warn + @deprecated) that lazily build a MustacheDialogRenderer for legacy callers; removal version computed from VERSION_MAJOR. A missing .dialog file still falls back to the dialog name with dots replaced by spaces (eg "record.not.found" -> "record not found"), the documented contract of the previous renderer. Unfilled slots now raise ovos_spec_tools.UnfilledSlot instead of being silently dropped. BREAKING CHANGE: dialog rendering is now performed by ovos_spec_tools.render. The dialog_renderer properties on SkillResources and OVOSSkill are deprecated compat shims. A dialog phrase with a {name} slot that has no value now raises UnfilledSlot instead of producing a partially-rendered string. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
DialogFile.render / IntentFile.render now raise FileNotFoundError when the resource file is missing or empty, instead of silently returning the resource name with dots replaced by spaces. OVOSSkill.render_dialog stays polymorphic: speak_dialog / get_response accept either a dialog key or a literal utterance, so a name with no matching .dialog file is caught and returned verbatim as the utterance. The silent dialog-name humanization is gone — a genuinely missing file is now an error at the layer that owns the file. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Mark the entire resource_files module as deprecated. It predates the OVOS formal specifications and is considered overengineered. Everything stays fully functional for downstream consumers; nothing is removed. All public names (ResourceType, ResourceFile and subclasses, SkillResources, CoreResources, UserResources, RegexExtractor, locate_base_directories, locate_lang_directories, find_resource) now carry the @deprecated decorator plus an inner warnings.warn and a .. deprecated:: docstring notice. Messages point downstream to the ovos-spec-tools replacements where they exist (LocaleResources, render/DialogRenderer, expand, closest_lang/ standardize_lang); legacy types with no spec replacement (.qml, .json, .list, .value, .rx, .word, .template) are flagged as not part of the OVOS formal specifications. To avoid the framework spamming deprecation warnings at itself (these classes are built per-skill, per-language on hot paths), a _caller_is_internal stack guard suppresses both the log notice and the DeprecationWarning when the caller is ovos_workshop's own code; the warnings remain visible to downstream callers. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…gh ovos-spec-tools OVOSSkill and the rest of ovos_workshop no longer import or call ``ovos_workshop.resource_files``. The 6 OVOS-spec resource roles (.intent / .dialog / .entity / .voc / .blacklist / .prompt) are now resolved through ``ovos_spec_tools.LocaleResources``; dialogs are rendered via ``ovos_spec_tools.render``; language directories are resolved via ``ovos_spec_tools.closest_lang``. The legacy resource types are removed from OVOSSkill: ``.rx`` / regex, ``.list``, ``.value``, ``.word``, ``.template``, ``.qml``, and skill-side ``.json``. The corresponding OVOSSkill methods (``load_regex_file*``, ``load_list_file``, ``load_named_value_file``, ``load_word_file``, ``load_template_file``, ``locate_qml_file``, ``load_json_file``, ``load_skill_regex``, ``find_resource``) and the file-based ``load_regex_files`` code path are gone. The string-based ``register_regex`` (adapt regex from a literal pattern) stays. ``OVOSSkill.resources`` now returns ``ovos_spec_tools.LocaleResources``. ``load_lang`` keeps a backwards-compatible signature but the ``lang`` argument is ignored (LocaleResources picks the language per-call). ``dialog_renderer`` is no longer backed by ``MustacheDialogRenderer`` — the deprecated property returns ``None`` and warns. ``ovos_workshop`` ships small JSON config files (``word_connectors.json`` / ``euphony.json``) under its own ``locale/`` directory; ``ovos_workshop.skills.util.join_word_list`` now reads them directly with ``closest_lang`` resolution, not through ``CoreResources``. Test changes: - ``test_base.py``/``test_ovos.py``: assertion on the type of ``OVOSSkill.resources`` updated from ``SkillResources`` to ``LocaleResources``. - ``test_base.py``: removed TODO stubs for the dropped methods (``find_resource``, ``load_regex_files``). Stacks on #412 / #411; part of OpenVoiceOS/architecture#7. BREAKING CHANGE: legacy resource-type support (``.rx``, ``.list``, ``.value``, ``.word``, ``.template``, ``.qml``, ``.json``) is removed from ``OVOSSkill``. Skills that registered file-based regex via ``load_regex_files`` / ``load_skill_regex`` or that called ``OVOSSkill.find_resource`` / the dropped ``load_*`` helpers must migrate to ``ovos_spec_tools.LocaleResources``. ``OVOSSkill.resources`` now returns ``ovos_spec_tools.LocaleResources`` instead of ``ovos_workshop.resource_files.SkillResources``. The 6 OVOS-spec resource roles continue to work unchanged. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
|
Warning Review limit reached
More reviews will be available in 37 minutes and 21 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR migrates ovos-workshop's resource handling from legacy SkillResources to ovos_spec_tools.LocaleResources, adds a legacy-deprecation shim, updates language-matching utilities, and expands unit and end-to-end tests for regex loading and UniversalSkill language-decoupling. ChangesLocale Resources Migration
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Restore partial back-compat for downstream skills that still call `self.resources.load_*` / `render_dialog` etc.: `self.resources` is now a deprecated property that returns a legacy SkillResources, with a DeprecationWarning pointing callers at the high-level skill methods (speak_dialog, register_intent_file, voc_match, ...) or at ovos_spec_tools.LocaleResources for low-level access. Workshop's own internal code goes through a private property, `OVOSSkill._locale_resources`, which returns the LocaleResources built by load_lang() — no functional change to the rewire, just a separate public-vs-internal handle. The SkillResources construction inside the deprecated property is silenced for workshop-internal callers by the _caller_is_internal guard from #412. Test assertions for `self.skill.resources` updated to check `self.skill._locale_resources` (the internal handle) for LocaleResources. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Restore the two OVOSSkill methods the rewire had removed, as deprecated-but-functional shims: - find_resource: delegates to ovos_workshop.resource_files.find_resource (itself deprecated, silenced for workshop-internal callers via the _caller_is_internal guard). - load_regex_files: constructs a SkillResources directly (legacy path) and registers the .rx files with the adapt intent service, exactly as before. .rx is not part of the OVOS formal specifications, but skills shipping it still work through this shim. Both emit a DeprecationWarning and log via @deprecated. UniversalSkill ._load_lang was an empty dead override and stays dropped. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds a dedicated regression suite around .rx regex resource loading so the deprecation timeline for legacy adapt-style regex intents stays under CI control even though the rest of resource_files was dropped. - unittests/skills/test_regex_loading.py: pins OVOSSkill.load_regex_files behaviour — silent on no-.rx trees, registers patterns with the alphanumeric skill-id prefix, emits a DeprecationWarning recommending padatious-style intents only when a file is actually loaded, logs the loaded path, raises on malformed patterns. - end2end/test_regex_skill.py: ovoscope-driven E2E test — a skill ships a .rx file, an adapt IntentBuilder requires the regex's named group, inject_message of `recognizer_loop:utterance` causes the intent handler to fire and emit `speak`. This trips CI the day regex support breaks or is silently removed. Also fixes load_regex_files locale discovery: walks locale/ subdirs and matches each subdir name against native_langs via closest_lang, so an en-US/ tree is picked up by a skill that declares only `en` as native (previous code joined native_langs to locale/, missing region-tagged subdirs). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
26 tasks
The hand-rolled locale-subdir walk in load_regex_files (standardize_lang on each subdir name + closest_lang against native_langs) is exactly what iter_locale_dirs encapsulates. Drop the duplicated logic. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- pyproject: pin ovos-spec-tools>=0.1.0a1 (iter_locale_dirs / lang_matches). - app.py, converse.py, ovos.py: replace every standardize_lang_tag with ovos_spec_tools.standardize_lang and drop the ovos_utils.lang import. - skills/util.py + skills/ovos.py: collapse the duplicated locale-dir walk (list locale/, closest_lang, match best back to dir) into iter_locale_dirs + closest_lang from ovos-spec-tools. resource_files.py keeps its own walk — the whole module is already deprecated and scheduled for removal. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Earlier commits in this PR appended deprecation shims after their neighbours, swapping find_resource and load_regex_files and sliding load_lang past both. Restore the original layout — load_lang group, load_regex_files, find_resource, then the new _locate_lang_file helper — so the PR diff against origin/dev reads as additions/edits, not as moves. Pure code-move commit; symmetric 87/87 line swap.
Bump the pin to >=0.2.0a1 and delegate _locate_lang_file to LocaleResources.find. The override-precedence + closest-language walk now lives in one place; workshop just keeps the str/Path adaptation. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ources at __init__ dialog_renderer was reduced to returning None, breaking callers that still do self.dialog_renderer.render(name, data). Restore the legacy MustacheDialogRenderer (lazily built and reused via the same _skill_resources_compat cache that backs self.resources) — the deprecation message is unchanged, the return shape is restored. _locale_resources was a property recomputing LocaleResources(self.res_dir) on every access. The skill's res_dir is fixed at __init__, so bind the instance there once and drop the property. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
self.resources, self.dialog_renderer and self.find_resource are kept for one release while skills migrate to the spec-tools-backed path. Move all three — and the _skill_resources_compat cache they share — into ovos_workshop.skills._legacy_resources, which OVOSSkill inherits. Ends the deprecation cycle in one removal: drop the mixin from OVOSSkill's bases and delete the file. Nothing on OVOSSkill itself references the legacy SkillResources path any more. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ixin ovos-core deprecated runtime_requirements (network_requirements has been deprecated for years). Move both classproperties into _LegacyResourcesMixin so the whole 'pre-spec-tools' surface of OVOSSkill — resources, dialog_renderer, find_resource, runtime_requirements, network_requirements — disappears as one removal: drop the mixin from the bases and delete the file. Subclass overrides keep working: skill_launcher accesses the property via hasattr/MRO so it does not care whether it lives on OVOSSkill itself or on a base. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Dialogs are loaded lazily by LocaleResources at render time, so the old load_dialog_files entry point has nothing to do. Move the backwards-compat no-op into the mixin alongside the other deprecated surface — it disappears with the rest when the mixin is dropped. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- pyproject: bump pin to >=0.3.0a1 (vocabulary_keywords, utterance_contains, strip_samples). - load_vocab_files collapses from 17 lines to a 4-line loop over LocaleResources.vocabulary_keywords — also fixes a latent regression where each expanded sample was registered as its own (entity, []) pair, losing the OVOS-INTENT-2 §4.3 canonical/alias distinction. - voc_match delegates the match logic to utterance_contains; the workshop method keeps the ensure_ascii single-flag back-compat (forwards to both strip_diacritics and strip_punct). - remove_voc delegates to strip_samples — whole-word, longest-first. Drops the local ovos_utils.text_utils.remove_accents_and_punct import and the inline regex match/strip code now living in spec-tools. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Bump pin to >=0.4.0a1 and route voc_list through LocaleResources.voc_list — which returns [] for a missing resource — so voc_match no longer needs the FileNotFoundError/log-warning plumbing and drops to a single utterance_contains call. remove_voc keeps its strip_samples one-liner. The per-skill _voc_cache stays — caching is an instance concern (some skills are short-lived, some long), and the spec-tools loader has no notion of it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…imports voc_match_cache is a back-compat accessor for the per-skill _voc_cache dict; its setter already self-identified as deprecated. Move both property and setter into the mixin so they disappear with the rest of the legacy surface, and replace the stringly-typed log.warning with a real DeprecationWarning. Also drop now-unused imports from ovos.py: classproperty (only consumer was runtime_requirements, which moved with the previous commit) and isdir (only consumer was the hand-rolled locale-dir walk, now folded into iter_locale_dirs). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
UniversalSkill authors .dialog / .voc / .intent / .entity files in internal_language; intent handlers translate utterances into that language and speak() translates back to the query lang. Pre-spec-tools this worked through SkillResources(lang=internal_language); the removal of _load_lang left every resource lookup defaulting to the query lang instead — dialogs unresolved, voc_match silently False. Introduce a single extension point on OVOSSkill — the _resource_lang property, defaulting to self.lang. Every resource lookup on the skill (render_dialog, voc_list, voc_match, remove_voc, the adapt keyword/regex registrar) now consumes this property instead of going direct to self.lang. UniversalSkill overrides _resource_lang to return self.internal_language; the lang-routing fix is one property, the entire resource surface follows. Add an ovoscope tripwire (test/end2end/test_universal_skill.py) covering the three legs of the contract: * _resource_lang returns internal_language on a UniversalSkill; * speak_dialog renders the en-US .dialog when the message's query lang is es-ES; * voc_match matches the en-US affirmative.voc when the query lang is fr-FR. The test stubs translate_utterance / translate_message on the skill class because the ovoscope env has no translator plugin and we want to pin the internal-language phrasing anyway. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two long-latent bugs surface as soon as a UniversalSkill round-trip is actually exercised in CI: * UniversalSkill.speak called translate_utterance(text, sauce_lang, out_lang) — target / source positional args swapped. The result translated FROM the user's language TO internal, the wrong direction for an outbound reply, so English-only skills (Wolfram-style, animal facts, weather API) silently leaked English text to non-English users. Pass the args by name so the swap can not recur. * translate_message indexed message.data['__tags__'] unconditionally; __tags__ is set by the adapt pipeline and absent under padacioso / padatious / direct event handlers, so any UniversalSkill receiving a non-adapt intent raised KeyError before the handler ran. Guard with a key-presence check. Expand the ovoscope tripwire (test/end2end/test_universal_skill.py) into a full Wolfram-style round-trip: the test skill ships an English .intent file, exposes a fact.intent event for an English-only fake API, and is invoked with a Portuguese slot capture. The assertion chain pins (a) the .intent file was located via the en-US fixture tree, (b) the handler saw the slot folded into internal_language by translate_message before the callback fired, (c) the English API reply came back in Portuguese via UniversalSkill.speak. translate_keys on the test skill is extended to include the 'animal' slot so the fold-in step covers the captured entity, not just the raw utterance. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Replace the synthetic intent-event injection with a real
recognizer_loop:utterance addressed to an English-only skill in
Portuguese. The skill ships pt-PT/fact.intent alongside en-US/, the
minicroft is configured with native_langs covering both, padacioso
matches the pt-PT template, the universal_intent_handler wrapper
folds the captured 'gatos' slot into internal_language ('cats') and
the handler runs purely in English (hits the English-only fake API,
returns an English fact). UniversalSkill.speak translates the reply
back to Portuguese for the user — the English internals stay hidden,
which is the point of the skill class.
Three failure modes the test covers:
* the pt-PT .intent does not register → no match, no speak;
* the inbound translation step is skipped → handler sees 'gatos',
API misses, user gets the fallback;
* the outbound translation step is skipped or arg-swapped → user
gets the English fact verbatim.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…gion PR OpenVoiceOS/ovos-bus-client#213 (merged into 2.0.0a4) migrated Session.deserialize off ovos_utils.lang.standardize_lang_tag's macro=True default, so a session lang of 'pt-PT' is no longer folded to 'pt' on the way through the bus. Bump the workshop minimum so the correct behaviour is guaranteed. Keep the translation stub in the UniversalSkill ovoscope test resilient to either bus-client release: fold lang tags to their primary subtag at lookup time. The note in _FAKE_TRANSLATIONS now points at the bus-client version that fixed the strip, not at a fixture-side workaround. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
A fresh review of the PR found a subtle regression: __init__ was
constructing the bound LocaleResources from skill_locale alone
(LocaleResources(self.res_dir)), passing res_dir where the
constructor expects the skill's locale/ directory. load_lang() built
the proper three-source chain — user-data, skill, workshop core —
but every call site reading self._locale_resources (render_dialog,
voc_match, _locate_lang_file) went through the truncated instance.
Concrete impact:
* self.voc_match('cancel') would have failed to find workshop's own
locale/en-US/cancel.voc — every skill relying on the core vocab
(cancel.voc, skill.error.dialog, game_pause.dialog, …) silently
broke;
* user resource overrides under <xdg>/resources/<skill_id>/ never
resolved.
Build the bound instance with the same three sources load_lang() uses
and cache it under res_dir so load_lang(self.res_dir) returns the
same object — single source of truth, no divergence between the two
paths. Drop the now-redundant self._lang_resources = {} init.
Relocate the universal_locale fixture from flat <lang>/ to
locale/<lang>/ so it follows the OVOS-INTENT-2 §2.1 layout that
LocaleResources documents. The pre-review code path accidentally
worked with the flat layout because of the same skill_locale=res_dir
bug; with the bug fixed, the fixture must follow the spec.
Refresh the _LegacyResourcesMixin docstring — the file already grew
beyond the three properties the docstring claimed; it now lists every
surface in the mixin.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
The code stars have aligned. Here's the update. ✨I've aggregated the results of the automated checks for this PR below. 📋 Repo HealthA detailed health report for the project. 📝 ✅ All required files present. Latest Version: ✅ 🔍 LintChecking the alignment of your contribution. 📏 ❌ ruff: issues found — see job log 🔨 Build TestsI tried building your changes, and here's what happened! 🔨
❌ 3.10: Install OK, tests failed 🔒 Security (pip-audit)Ensuring our digital fortress remains impenetrable. 🏰 ✅ No known vulnerabilities found (68 packages scanned). ⚖️ License CheckScanning for any potential trademark infringements. ™️ ✅ No license violations found (49 packages). License distribution: 12× MIT License, 9× Apache Software License, 7× MIT, 6× Apache-2.0, 2× BSD-3-Clause, 2× ISC License (ISCL), 2× PSF-2.0, 2× Python Software Foundation License, +7 more Full breakdown — 49 packages
Copyright (c) 2022 Phil Ewels Permission is hereby granted, free of charge, to any person obtaining a copy The above copyright notice and this permission notice shall be included in all THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR Policy: Apache 2.0 (universal donor). StrongCopyleft / NetworkCopyleft / WeakCopyleft / Other / Error categories fail. MPL allowed. Processing... Done! Have a productive day! ☕ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
ovos_workshop/skills/ovos.py (1)
612-612: 💤 Low valueRegex named-group prefixing may produce invalid group names.
The prefixing logic on line 634 produces
(?P<skillid_(?P<name>when splitting on(?P<and joining with the unique prefix. This looks correct for transforming(?P<name>...)into(?P<skillid_name>...). However, theunique_prefixon line 612 is"(?P<" + self.alphanumeric_skill_id(no trailing underscore), so a pattern(?P<foo>...)becomes(?P<skillid_foo>...)which concatenates the skill ID directly with the group name.If
alphanumeric_skill_idends with a digit and the group name starts with a digit, this could produce an invalid Python regex group name (group names must start with a letter or underscore). Consider adding an underscore separator.Proposed fix
- unique_prefix = "(?P<" + self.alphanumeric_skill_id + unique_prefix = "(?P<" + self.alphanumeric_skill_id + "_"Also applies to: 634-634
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@ovos_workshop/skills/ovos.py` at line 612, The current unique_prefix construction uses "(?P<" + self.alphanumeric_skill_id without a separator which can create invalid regex group names when concatenated; update the unique_prefix used when splitting/joining on "(?P<" (referenced as unique_prefix and the split/join logic around self.alphanumeric_skill_id) to insert a single underscore between the skill id and the original group name (e.g. ensure unique_prefix becomes "(?P<" + self.alphanumeric_skill_id + "_" but avoid double underscores by only adding the underscore if self.alphanumeric_skill_id does not already end with one).test/end2end/test_universal_skill.py (1)
83-93: ⚡ Quick winPrefer passing
internal_languageviakwargsbeforesuper().__init__for safer arg binding.This avoids brittle binding when positional args are present and keeps the constructor robust to upstream call-shape changes.
Proposed refactor
def __init__(self, *args, **kwargs): # resources_dir is honoured at __init__ time (before _startup # runs initialize); pass it through so register_intent_file # finds fact.intent in the fixture tree. kwargs.setdefault("resources_dir", _FIXTURE_DIR) # The handler reads a captured ``animal`` slot; tell # translate_message to fold that key into internal_language too, # alongside the default ``utterance`` / ``utterances`` keys. kwargs.setdefault("translate_keys", ["animal", "utterance", "utterances"]) - super().__init__(internal_language="en-US", *args, **kwargs) + kwargs["internal_language"] = "en-US" + super().__init__(*args, **kwargs)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/end2end/test_universal_skill.py` around lines 83 - 93, The constructor currently passes internal_language as a positional/keyword arg to super().__init__ which can be brittle; instead set kwargs["internal_language"] = "en-US" (or kwargs.setdefault("internal_language", "en-US")) before calling super().__init__, preserve the existing kwargs.setdefault("resources_dir", _FIXTURE_DIR") and kwargs.setdefault("translate_keys", ["animal", "utterance", "utterances"]) logic, then call super().__init__(*args, **kwargs) so binding is stable; update the __init__ method accordingly (refer to __init__, resources_dir, translate_keys, and the super().__init__ call).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pyproject.toml`:
- Line 16: The dependency constraint for "ovos_bus_client>=2.0.0a4" is too loose
and may pull incompatible 2.x/3.x releases; update the version specifier in
pyproject.toml for the "ovos_bus_client" entry to a bounded range such as
">=2.0.0a4,<2.0.0" (or mirror ovos-workshop’s constraint like "<2.0.0,>=1.3.8a1"
if you prefer exact alignment) so the package manager cannot install
incompatible major releases.
In `@test/unittests/skills/test_regex_loading.py`:
- Around line 79-81: The assertion's warning filter is checking for "regex" but
elsewhere the test expects a "padatious" deprecation; update the list
comprehension that builds the filtered warnings (the expression using caught and
issubclass(w.category, DeprecationWarning)) to check for "padatious" in
str(w.message).lower() instead of "regex" (to make the filter match the actual
deprecation text being asserted later).
- Around line 38-45: Tests overwrite the XDG_CONFIG_HOME env var but always
remove it in tearDownClass, which can delete a pre-existing value; modify
setUpClass to save the prior os.environ.get("XDG_CONFIG_HOME") (e.g., store on
cls as cls._old_xdg_config_home) before setting cls._tmp_config, and update
tearDownClass to restore the original value: if cls._old_xdg_config_home is None
remove the env var, otherwise set os.environ["XDG_CONFIG_HOME"] back to
cls._old_xdg_config_home; reference the setUpClass/tearDownClass methods and
cls._tmp_config/cls._old_xdg_config_home when making the change.
---
Nitpick comments:
In `@ovos_workshop/skills/ovos.py`:
- Line 612: The current unique_prefix construction uses "(?P<" +
self.alphanumeric_skill_id without a separator which can create invalid regex
group names when concatenated; update the unique_prefix used when
splitting/joining on "(?P<" (referenced as unique_prefix and the split/join
logic around self.alphanumeric_skill_id) to insert a single underscore between
the skill id and the original group name (e.g. ensure unique_prefix becomes
"(?P<" + self.alphanumeric_skill_id + "_" but avoid double underscores by only
adding the underscore if self.alphanumeric_skill_id does not already end with
one).
In `@test/end2end/test_universal_skill.py`:
- Around line 83-93: The constructor currently passes internal_language as a
positional/keyword arg to super().__init__ which can be brittle; instead set
kwargs["internal_language"] = "en-US" (or kwargs.setdefault("internal_language",
"en-US")) before calling super().__init__, preserve the existing
kwargs.setdefault("resources_dir", _FIXTURE_DIR") and
kwargs.setdefault("translate_keys", ["animal", "utterance", "utterances"])
logic, then call super().__init__(*args, **kwargs) so binding is stable; update
the __init__ method accordingly (refer to __init__, resources_dir,
translate_keys, and the super().__init__ call).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4476971c-a5a7-496b-9fd7-55262cac9467
📒 Files selected for processing (20)
.gitignoreovos_workshop/app.pyovos_workshop/resource_files.pyovos_workshop/skills/_legacy_resources.pyovos_workshop/skills/auto_translatable.pyovos_workshop/skills/converse.pyovos_workshop/skills/ovos.pyovos_workshop/skills/util.pypyproject.tomltest/end2end/__init__.pytest/end2end/locale/en-US/play.rxtest/end2end/test_regex_skill.pytest/end2end/test_universal_skill.pytest/end2end/universal_locale/locale/en-US/affirmative.voctest/end2end/universal_locale/locale/en-US/echo.dialogtest/end2end/universal_locale/locale/en-US/fact.intenttest/end2end/universal_locale/locale/pt-PT/fact.intenttest/unittests/skills/test_base.pytest/unittests/skills/test_ovos.pytest/unittests/skills/test_regex_loading.py
…k as a property
Two follow-ups to the review fixes:
1. _locale_resources is now a property delegating to load_lang(self.res_dir)
rather than an attribute bound at __init__. test_base mutates res_dir
on a constructed skill (the legitimate pattern for fixture-driven tests
and live-reload skills), and the init-time binding silently swallowed
those mutations. load_lang caches per root_directory, so the property
stays O(1) on the hot path; res_dir changes are picked up on the
next access.
2. test/unittests/skills/test_legacy_resources.py — 23 new tests pinning
three layers of the contract this PR establishes:
* OVOSSkill resource plumbing — _locale_resources is a LocaleResources
instance, picks up res_dir mutation, threads the workshop-core
locale source (catches the bug the earlier review uncovered);
render_dialog renders from .dialog files and returns its argument
verbatim on miss; _resource_lang defaults to self.lang and
subclass overrides propagate to voc_list lookups.
* _LegacyResourcesMixin shims — self.resources / self.dialog_renderer
/ self.find_resource / self.voc_match_cache setter / self.load_dialog_files
all keep working, emit DeprecationWarning, and share one cached
SkillResources so they do not drift.
* Legacy file formats in the wild — .qml, .json, .list, .value,
.word, .template still load through the deprecated self.resources
accessor; .qml resolves through self.find_resource. No OVOS-INTENT-2
replacement exists for these so back-compat through the mixin is
the contract.
Plus one §2.1 precedence test: a user file under
<xdg-data>/resources/<skill_id>/<lang>/cancel.voc overrides workshop's
bundled core cancel.voc.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…omments pyproject.toml: revert ovos_bus_client to >=1.3.8a1,<2.0.0 — ovos-plugin-manager 2.4.x requires <2.0.0 so the >=2.0.0a4 lower-bound made the dep graph unresolvable, breaking all CI install steps across every Python version. test/end2end/test_universal_skill.py: - update comment to not claim the pin requires a specific bus-client version - move internal_language into kwargs before super().__init__ call for robust positional arg binding (CodeRabbit actionable) test/unittests/skills/test_regex_loading.py: - preserve pre-existing XDG_CONFIG_HOME in setUpClass/tearDownClass so a real value already in the environment is restored rather than deleted (CodeRabbit actionable) - use "padatious" instead of "regex" in the no-warning filter to match the actual deprecation message text, consistent with the positive assertion below (CodeRabbit actionable) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
JarbasAl
changed the title
…k requirements shims Floor bumps: - ovos-utils: 0.7.0 -> 0.11.1a1 (region-preserving standardize_lang_tag fix #377; without it OVOSAbstractApplication.get_language_dir and the 10+ standardize_lang_tag(macro=True) call sites in ovos.py silently strip the region from en-US et al, which breaks per-region locale lookups and final-session equality checks downstream). - ovos-spec-tools: 0.4.0a1 -> 0.5.1a1 (the release that ships LocaleResources.find/load_blacklist/load_vocabulary semantics this PR now relies on, plus the empty-msg_type construction fix). Adds the matching <1.0.0 upper bound to align with house style. Tests: - TestRuntimeRequirementsShim — three cases covering the classproperty shim that was added to _LegacyResourcesMixin without dedicated coverage: default returns a RuntimeRequirements, subclass override is honoured (the LAN/cache/offline pattern that historic skills use), network_requirements legacy alias delegates correctly. 26 passed in the legacy-resources suite (was 23). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…t-file registration
Two new ovoscope tests covering the highest-risk silent-failure
paths the spec-tools migration introduces:
- test/end2end/test_speak_dialog.py — drives the
speak_dialog -> render_dialog -> LocaleResources.load_dialog ->
ovos_spec_tools.render bus chain. Fixture greet.dialog uses both a
{name} slot AND a <salutation> voc reference, so a clean render
proves slot substitution AND voc-reference resolution AND
vocabularies() loading all work end-to-end. If any link broke the
speak message would carry the raw template (or the dialog
filename); the assert pins both substitutions.
- test/end2end/test_padatious_intent_file.py — drives
register_intent_file -> _locate_lang_file (the new private path
resolver) -> padatious registration -> bus match. _locate_lang_file
is brand new in this PR; a silent miss returns None and padatious
registers nothing — the intent never fires and the test catches it.
Pipeline pinned to padatious so the failure mode is unambiguous;
skipped via ovoscope.is_pipeline_available when padatious isn't
installed in the test env.
Local: speak_dialog test passes; padatious test skips (no padatious
in the workshop venv). CI installs padatious (require_padatious=true
on the ovoscope workflow) so both will run.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…tch coverage
Padacioso is always available (pure Python, no swig/fann), so the
intent-file e2e test no longer needs a skip-guard or
`require_padatious` on the workflow.
- test_padacioso_intent_file.py — renamed from
test_padatious_intent_file.py; same skill + utterance + assertion,
pipeline pinned to padacioso. Always runs.
- test_padacioso_entity_file.py (new) — exercises
register_entity_file end-to-end. Skill registers
`drink.entity` (coffee/tea/juice) + `order.intent` with a
{drink} slot; injects "i want a coffee" and asserts the
handler's echo carries `coffee` in the speak — proves the
`.entity` resource resolved through `_locate_lang_file` and
padacioso constrained the slot fill correctly. If the entity
registration is silently dropped, the bare {drink} slot either
fails to match or captures the wrong span.
- test_voc_match.py (new) — exercises
`OVOSSkill.voc_match` end-to-end against
`locale/en-US/yes.voc`. Two assertions: in-vocabulary phrase
returns True, out-of-vocabulary returns False. Pins the rewired
`voc_match -> voc_list -> LocaleResources.load_vocabulary ->
_voc_cache` chain — the workhorse keyword helper used by
stop/cancel/confirm flows across the ecosystem.
Dropped a get_response sketch — `get_response` activates listening
and requires more session state than the test rig drives cleanly;
the underlying render_dialog chain is already covered end-to-end
by test_speak_dialog.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…le role One fixture skill that ships every legacy resource file type the SkillResources surface still understands, and one initialize() that calls every deprecated entry point on _LegacyResourcesMixin. If any one shim regresses, the omnibus skill fails to construct or its initialize() raises — failure mode is unambiguous and the bisect target is one file. Covers, in one skill: - Class-level shims: runtime_requirements override (offline-skill pattern), network_requirements alias. - Property shims: resources (legacy SkillResources handle), dialog_renderer (legacy MustacheDialogRenderer), voc_match_cache (getter + external-mutation setter — distinct call paths, distinct deprecation warnings). - Method shims: load_dialog_files no-op, find_resource. - Legacy resource roles via self.resources.load_*_file: .list, .value, .template, .word, .json, .voc (vocabulary). - UI resource via find_resource: .qml outside the locale tree. 13 assertions in one TestCase: one per surface, plus one that re-runs the omnibus in a recorded-warnings context and pins each expected DeprecationWarning substring (4 of them — resources, dialog_renderer, find_resource, voc_match_cache setter). Note on the fixture pattern: passing bus + skill_id to OVOSSkill triggers _startup which calls initialize, so resources_dir MUST be a constructor kwarg (post-init res_dir mutation is too late and SkillResources caches against the wrong path). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…t slot fill
.entity files in OVOS are training-data hints for the intent engine,
not strict slot constraints — an open {slot} captures any token even
without a sibling .entity. The previous test asserted on the slot
fill (`'i want a coffee'` -> handler echoing 'coffee'), which would
pass even if register_entity_file did nothing.
Rewrite to assert on the registration plumbing instead — the part
that actually exercises _locate_lang_file for .entity:
- Skill registers drink.entity on a bus trigger (so the test can
subscribe BEFORE the registration fires).
- Test subscribes to padatious:register_entity, fires the trigger.
- Asserts file_name ends with drink.entity (proves
_locate_lang_file resolved the .entity file to a real path).
- Asserts samples == [coffee, juice, tea] (proves the OVOS-side
entity reader opened the resolved path and parsed it).
Drops the now-unused order.intent fixture and tightens imports.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
test/unittests/skills/test_legacy_skill_omnibus.py (1)
246-252: ⚡ Quick winInclude
load_dialog_filesin deprecation-warning assertions to match the omnibus claim.
initialize()explicitly exercisesload_dialog_files(), but this warning test does not assert its deprecation signal, leaving a coverage hole in the “every legacy entry point” tripwire.Suggested patch
for needle in ("self.resources is deprecated", + "load_dialog_files", "dialog_renderer is deprecated", "find_resource is deprecated", "voc_match_cache external mutation is deprecated"):🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@test/unittests/skills/test_legacy_skill_omnibus.py` around lines 246 - 252, The test's deprecation assertion loop omits the warning for load_dialog_files even though initialize() calls it; update the assertions in test_legacy_skill_omnibus.py to include "load_dialog_files is deprecated" in the tuple of needles being checked so the test verifies that load_dialog_files() emits a DeprecationWarning (reference the initialize() call and the load_dialog_files symbol to locate where the messages list is checked).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@test/end2end/test_padacioso_entity_file.py`:
- Around line 68-70: The test currently sets the completion gate unconditionally
when any "padatious:register_entity" message arrives; modify the _on_register
callback to inspect msg.data and only call registered.set() and update
registration when the payload matches the expected entity attributes (e.g.,
expected entity name/type/intents) to avoid racing with unrelated bus traffic;
apply the same conditional gating change to the other handler referenced around
lines 82-87 so both handlers validate msg.data before marking the test complete.
In `@test/end2end/test_padacioso_intent_file.py`:
- Around line 65-67: The on_speak handler currently sets speak_event for any
speak message, causing races; update the on_speak function (the callback named
on_speak that appends to seen_speaks and calls speak_event.set()) to first
extract the utterance via msg.data.get("utterance", "") and only call
speak_event.set() when that utterance matches the expected text for the test
(e.g., equals or contains the expected_utterance variable); still append the
utterance to seen_speaks for logging, but guard speak_event.set() behind the
match. Apply the same change to the second speaker handler in the block that
mirrors lines 83-90.
In `@test/end2end/test_speak_dialog.py`:
- Around line 64-67: The on_speak handler currently sets speak_event on the
first speak message and appends any utterance to seen_speaks; change it to only
append and set speak_event when the payload matches the expected utterance(s)
(e.g., inspect msg.data.get("utterance") and compare to the expected greeting),
so unrelated speak traffic won't trigger the test; update the on_speak function
and the corresponding second handler (the block around lines 79-91) to perform
this exact-match (or allowed-set) check before calling speak_event.set() and
adding to seen_speaks.
In `@test/unittests/skills/test_legacy_skill_omnibus.py`:
- Around line 170-173: tearDownClass currently calls cls.fixture.cleanup()
without stopping the class-scoped skill started in setUpClass; call the skill's
shutdown/stop method (the instance of _LegacyOmnibusSkill created in setUpClass)
before deleting fixture files so bus handlers and internal state are torn down.
Locate the class-level skill instance created in setUpClass (referenced as
_LegacyOmnibusSkill or whatever class attribute holds the skill) and invoke its
shutdown/stop/close method (e.g., skill.shutdown() or skill.stop()) prior to
calling cls.fixture.cleanup() in tearDownClass.
---
Nitpick comments:
In `@test/unittests/skills/test_legacy_skill_omnibus.py`:
- Around line 246-252: The test's deprecation assertion loop omits the warning
for load_dialog_files even though initialize() calls it; update the assertions
in test_legacy_skill_omnibus.py to include "load_dialog_files is deprecated" in
the tuple of needles being checked so the test verifies that load_dialog_files()
emits a DeprecationWarning (reference the initialize() call and the
load_dialog_files symbol to locate where the messages list is checked).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1c47810a-7a4e-406e-bc1b-6d73222b4801
📒 Files selected for processing (10)
test/end2end/locale/en-US/drink.entitytest/end2end/locale/en-US/greet.dialogtest/end2end/locale/en-US/salutation.voctest/end2end/locale/en-US/wave.intenttest/end2end/locale/en-US/yes.voctest/end2end/test_padacioso_entity_file.pytest/end2end/test_padacioso_intent_file.pytest/end2end/test_speak_dialog.pytest/end2end/test_voc_match.pytest/unittests/skills/test_legacy_skill_omnibus.py
✅ Files skipped from review due to trivial changes (5)
- test/end2end/locale/en-US/salutation.voc
- test/end2end/locale/en-US/wave.intent
- test/end2end/locale/en-US/greet.dialog
- test/end2end/locale/en-US/yes.voc
- test/end2end/locale/en-US/drink.entity
Comment on lines
+68
to
+70
| def _on_register(msg): | ||
| registration.update(msg.data) | ||
| registered.set() |
Contributor
There was a problem hiding this comment.
Gate completion on the expected entity registration payload.
Line 68-Line 70 currently marks the test complete on the first padatious:register_entity message. That can race with unrelated bus traffic and make this test flaky.
✅ Suggested fix
def _on_register(msg):
- registration.update(msg.data)
- registered.set()
+ data = msg.data or {}
+ file_name = data.get("file_name", "")
+ if file_name.endswith("drink.entity"):
+ registration.update(data)
+ registered.set()Also applies to: 82-87
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/end2end/test_padacioso_entity_file.py` around lines 68 - 70, The test
currently sets the completion gate unconditionally when any
"padatious:register_entity" message arrives; modify the _on_register callback to
inspect msg.data and only call registered.set() and update registration when the
payload matches the expected entity attributes (e.g., expected entity
name/type/intents) to avoid racing with unrelated bus traffic; apply the same
conditional gating change to the other handler referenced around lines 82-87 so
both handlers validate msg.data before marking the test complete.
Comment on lines
+65
to
+67
| def on_speak(msg): | ||
| seen_speaks.append(msg.data.get("utterance", "")) | ||
| speak_event.set() |
Contributor
There was a problem hiding this comment.
Filter speak events to the expected utterance before setting the wait event.
Line 65-Line 67 currently sets speak_event on any speak message. This can cause false negatives if unrelated speech arrives first.
✅ Suggested fix
def on_speak(msg):
- seen_speaks.append(msg.data.get("utterance", ""))
- speak_event.set()
+ utterance = msg.data.get("utterance", "")
+ seen_speaks.append(utterance)
+ if "waving back" in utterance:
+ speak_event.set()Also applies to: 83-90
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/end2end/test_padacioso_intent_file.py` around lines 65 - 67, The
on_speak handler currently sets speak_event for any speak message, causing
races; update the on_speak function (the callback named on_speak that appends to
seen_speaks and calls speak_event.set()) to first extract the utterance via
msg.data.get("utterance", "") and only call speak_event.set() when that
utterance matches the expected text for the test (e.g., equals or contains the
expected_utterance variable); still append the utterance to seen_speaks for
logging, but guard speak_event.set() behind the match. Apply the same change to
the second speaker handler in the block that mirrors lines 83-90.
Comment on lines
+64
to
+67
| def on_speak(msg): | ||
| seen_speaks.append(msg.data.get("utterance", "")) | ||
| speak_event.set() | ||
|
|
Contributor
There was a problem hiding this comment.
Wait for the expected speak payload, not just any speak event.
Line 64-Line 67 sets the event on the first speak message. If unrelated speak traffic appears, this test can fail before the greeting utterance is emitted.
✅ Suggested fix
def on_speak(msg):
- seen_speaks.append(msg.data.get("utterance", ""))
- speak_event.set()
+ utterance = msg.data.get("utterance", "")
+ seen_speaks.append(utterance)
+ if "Alice" in utterance and "welcome aboard" in utterance:
+ speak_event.set()Also applies to: 79-91
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/end2end/test_speak_dialog.py` around lines 64 - 67, The on_speak handler
currently sets speak_event on the first speak message and appends any utterance
to seen_speaks; change it to only append and set speak_event when the payload
matches the expected utterance(s) (e.g., inspect msg.data.get("utterance") and
compare to the expected greeting), so unrelated speak traffic won't trigger the
test; update the on_speak function and the corresponding second handler (the
block around lines 79-91) to perform this exact-match (or allowed-set) check
before calling speak_event.set() and adding to seen_speaks.
Comment on lines
+170
to
+173
| @classmethod | ||
| def tearDownClass(cls): | ||
| cls.fixture.cleanup() | ||
|
|
Contributor
There was a problem hiding this comment.
Shut down the class-scoped skill in tearDownClass before deleting fixture files.
setUpClass fully starts _LegacyOmnibusSkill; only deleting fixture.root can leave bus handlers/internal state alive past this test class. Call shutdown first for cleaner test isolation.
Suggested patch
`@classmethod`
def tearDownClass(cls):
- cls.fixture.cleanup()
+ try:
+ if hasattr(cls, "skill"):
+ cls.skill.default_shutdown()
+ finally:
+ cls.fixture.cleanup()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@test/unittests/skills/test_legacy_skill_omnibus.py` around lines 170 - 173,
tearDownClass currently calls cls.fixture.cleanup() without stopping the
class-scoped skill started in setUpClass; call the skill's shutdown/stop method
(the instance of _LegacyOmnibusSkill created in setUpClass) before deleting
fixture files so bus handlers and internal state are torn down. Locate the
class-level skill instance created in setUpClass (referenced as
_LegacyOmnibusSkill or whatever class attribute holds the skill) and invoke its
shutdown/stop/close method (e.g., skill.shutdown() or skill.stop()) prior to
calling cls.fixture.cleanup() in tearDownClass.
The hand-rolled exact-match short-circuit + closest_lang fallback collapses to one call now that the helper is public (ovos-spec-tools 0.6.0a1). - pyproject: bump ovos-spec-tools floor 0.5.1a1 -> 0.6.0a1. - app.py: replace the 30+ line implementation with a delegating one-liner; drop the local os.listdir / isdir walk and the closest_lang/standardize_lang case-fallback dance. Smart-fallback policy now lives in one place (OVOS-INTENT-2 §2.2) so workshop and any other consumer get the same behaviour. Signature, return type and case-handling all preserved — the helper covers exact-case match, case mismatch, bare-language requests, and minor regional fallback through the same predicate the hand-rolled version was approximating. 190 passed in the unit + e2e subset that exercises get_language_dir and its callers. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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.
Stacked on #412 (
feat/deprecate-resource-files) → #411. Part of the OVOS migration onto ovos-spec-tools (OpenVoiceOS/architecture#7).OVOSSkilland the rest ofovos_workshopnow route resource access throughovos_spec_tools.LocaleResourcesdirectly. The legacyovos_workshop.resource_files.SkillResourcesis no longer used internally but stays shipped and stays available on everyOVOSSkillthrough a deprecation mixin — every skill in the ecosystem keeps working.Back-compat surface preserved
All legacy entry points stay on
OVOSSkillvia the new_LegacyResourcesMixin. Each emits a singleDeprecationWarningpointing at the spec-tools-backed replacement and routes through the same cachedSkillResourcesinstance so behaviour is unchanged.self.resourcesSkillResources(lazily built once per skill).self.resources.load_dialog,render_dialog,load_vocab, … all keep working.speak_dialog,voc_match, …) or constructovos_spec_tools.LocaleResourcesdirectly.self.dialog_rendererMustacheDialogRendererfrom the cachedSkillResources.self.dialog_renderer.render(name, data)works unchanged.OVOSSkill.render_dialog(which delegates toovos_spec_tools.render).self.find_resource(name, dirname=None, lang=None)ovos_workshop.resource_files.find_resource.ovos_spec_tools.LocaleResources.find.self.load_dialog_files(root=None)LocaleResources; pre-loading is unnecessary.self.voc_match_cacheself._voc_cache(the live cache used byvoc_match/voc_list). Setter still accepts a dict and warns.voc_match/voc_listdirectly — they cache internally.runtime_requirements/network_requirementsclasspropertyshims returningRuntimeRequirements().network_requirementscontinues to log the legacy rename warning.The mixin is a single
importaway from removal — when the deprecation window closes, dropping_LegacyResourcesMixinfromOVOSSkill's bases (and deleting_legacy_resources.py) is the only change needed.Other resource roles the legacy
SkillResourcesexposes (.list,.value,.word,.template,.qml, skill-side.json) keep working through the preservedself.resourceshandle — workshop internals no longer consume them, but skills that do are unaffected.What changed internally (no public-surface impact)
.dialogrendering →LocaleResources.load_dialog+ovos_spec_tools.render(withvocabularies()for<voc>references). Drivesrender_dialog,speak_dialog,get_response, theskill.errorflow..voc→LocaleResources.vocabularies()(eager Adapt-keyword registration) andload_vocabulary(forvoc_list/voc_match/remove_voc)..intent/.entity→ file-path resolved via a privateOVOSSkill._locate_lang_filehelper (closest_lang+os.walk); the path is then passed tointent_service.register_padatious_intent/register_padatious_entity..rxregex files →OVOSSkill.load_regex_filesis kept and self-contained (does not touchresource_files); registration goes through adapt as before.OVOSAbstractApplication.get_language_dirusesclosest_lang+os.listdirdirectly.word_connectors.json/euphony.jsonare read withclosest_langresolution — noCoreResources.grep -rn 'resource_files\|SkillResources\|MustacheDialogRenderer\|ResourceFile\|RegexExtractor' ovos_workshop/matches only insideresource_files.pyand the legacy mixin.One private removal
UniversalSkill._load_lang— a dead override that constructed aSkillResources; never part of the public API (underscore-prefixed). The base-classload_lang(public, on everyOVOSSkill) now returns aLocaleResourcesinstead of aSkillResources, but the caller pattern is the same and the methods callers use (load_dialog,load_vocab, etc.) exist on both.Tests
test/unittests/skills/test_legacy_resources.py(new, 358 lines) — exercises every shim on the mixin, verifies the deprecation warnings fire, confirms each call still returns a working object.test/unittests/skills/test_base.py,test_ovos.py— importLocaleResourcesinstead ofSkillResourcesfor the new-path assertions.test_resource_files*— untouched (the module is still shipped and tested).test/end2end/test_regex_skill.py,test/end2end/test_universal_skill.py— new ovoscope end-to-end coverage for the .rx and UniversalSkill paths.ovos_yes_no_solver-related collection errors, unrelated to this PR).Migration path for skill authors
Skills migrate incrementally: each
DeprecationWarningpoints at the spec-tools-backed replacement. No skill needs to change in this release cycle. The mixin and the underlyingresource_filesmodule both have a one-major-release deprecation window.🤖 Generated with Claude Code
Summary by CodeRabbit
Deprecations
Improvements
Tests
Chores