Skip to content

refactor: route OVOSSkill resource loading through ovos-spec-tools - #414

Draft
JarbasAl wants to merge 6 commits into
devfrom
feat/drop-resource-files-usage
Draft

JarbasAl wants to merge 6 commits into
devfrom
feat/drop-resource-files-usage

Conversation

@JarbasAl

@JarbasAl JarbasAl commented May 25, 2026 •

Copy link
Copy Markdown
Member

🤖 Auto-generated by Claude Sonnet 5.5 (claude-sonnet-5-5) via Claude Code — NOT human-reviewed. Verify before acting.

Supersedes #413 (closed during squash). Part of the OVOS migration onto ovos-spec-tools (architecture#7).

OVOSSkill and the rest of ovos_workshop route resource access through ovos_spec_tools.LocaleResources. The legacy ovos_workshop.resource_files.SkillResources is not used internally. It stays shipped and stays available on every OVOSSkill through a deprecation mixin.

Needs from the reviewer: a merge. The dialogs the workshop speaks itself (skill_error, the four game dialogs and no_media_available) ship in all 47 locales through #737, so a game skill and the media player have a dialog in every language. This is a prerelease, so it targets alpha skills only; released skills pin a stable workshop. The change keeps the legacy entry points.

Compatibility with current skills

Measured with the same harness on dev and on this branch over 85 current skill trees: startup messages, every intent and entity registration, every dialog over 40 seeds, and voc_list and voc_match for each vocabulary. The results are identical. The harness does not cover the behaviour changes in the next section.

A live MiniCroft run with the real classes of date-time (de-DE) and hello-world (gl-ES) answers every utterance tried, as dev does.

Behaviour changes ruled by the owner

These are rulings of the repository owner. They are not back-compatible and the title does not claim it.

  1. A missing dialog raises. speak_dialog, render_dialog, SkillResources.render_dialog and the legacy dialog_renderer.render raise ovos_workshop.exceptions.DialogNotFound, a subclass of FileNotFoundError, when no dialog file exists for the name in the current language. The message names the dialog, the language and the directories searched. dev spoke the dialog name with dots read as spaces. OVOS-INTENT-2 §2: "a missing .dialog leaves the handler with nothing to speak for that response in that language". An exception in a handler takes the normal error path: the error dialog is spoken and mycroft.skill.handler.error is emitted.
  2. The workshop does not use the legacy renderer. OVOSSkill and every other module resolve resources through ovos_spec_tools.LocaleResources only. ovos_workshop.resource_files, self.resources, self.dialog_renderer, find_resource and the object load_lang() returns keep working for outside callers as deprecated shims. A source-level test fails if a module outside the shims imports them.
  3. A dialog the specification rejects as a whole. When some phrases are valid for the given data, those phrases are spoken and one WARNING names the file. When no phrase renders, DialogNotFound is raised with the file and the reason. A dialog with different slots in different phrases, such as date-time de-DE time.current.dialog, speaks the valid phrases.
  4. Prompts stay free text. get_response, ask_yesno, ask_selection and the first-run intro message document their prompt as "a dialog resource or string". A skill builds such a prompt at runtime, so no dialog file can exist for it. With no dialog file of that name the text is spoken. A dialog that exists but cannot render still raises.

Specification

Slot names are lowercase ASCII letters, digits and underscores (OVOS-INTENT-1 §3.4). A dialog with {DOWN}, { digits } or an accented slot name is malformed and raises. This branch does not change that.

A repeated slot in one phrase, and two consecutive slots, are allowed in a .dialog by the owner's ruling. ovos-spec-tools takes that change in OpenVoiceOS/ovos-spec-tools#188. Until it is released, a dialog with a repeated slot raises DialogNotFound, and this branch does not work around it.

Duplicate files

OVOS-INTENT-2 §2 makes a skill malformed when two files of one extension share a base name in one language tree. The workshop does not pick one of them: the resource is reported once with a WARNING and skipped. A duplicate .intent, .entity or .voc is not registered, and a duplicate .dialog raises DialogNotFound.

Not supported

The legacy dialog/<lang>/, vocab/<lang>/ and regex/<lang>/ layouts, and flat layouts without a language folder, are not supported. OVOS-INTENT-2 §2 defines one layout, locale/<lang>/.

Back-compat surface

All legacy entry points stay on OVOSSkill through _LegacyResourcesMixin. Each emits a DeprecationWarning and uses one cached SkillResources per resource root and language. The removal version is computed from VERSION_MAJOR.

Legacy member Behaviour Replacement
self.resources Real SkillResources. load_dialog_file, render_dialog, load_vocabulary_file, load_list_file, .value, .template, .word and .json keep working. High-level methods (speak_dialog, voc_match, ...) or ovos_spec_tools.LocaleResources.
self.load_lang(...) Returns a LocaleResources that also answers every SkillResources method (load_dialog_file, render_dialog, load_vocabulary_file, dialog_renderer, ...) with a deprecation warning. The LocaleResources methods.
self.dialog_renderer Real MustacheDialogRenderer, loading .dialog files from subfolders too. OVOSSkill.render_dialog.
self.find_resource(name, dirname=None, lang=None) Delegates to ovos_workshop.resource_files.find_resource. LocaleResources.find.
self.load_dialog_files(root=None) No-op. Drop the call.
self.voc_match_cache Live _voc_cache getter. The setter warns and accepts a dict. voc_match and voc_list cache internally.
runtime_requirements, network_requirements classproperty shims. Subclass overrides are honoured. Let the framework infer them from the intent surface.

load_lang(...).vocabularies() without a language still returns the SkillResources map, and load_vocabulary_file keeps the written order of each expansion. The shims read dialogs from subfolders as before.

What changes inside

Resource Path
.dialog LocaleResources.load_dialog, then ovos_spec_tools.render, with vocabularies() for <voc> references. See the behaviour changes above for a missing, empty, duplicated or rejected dialog.
.voc LocaleResources.vocabularies() for keyword registration. voc_list (lowercased, as before) for voc_match and remove_voc.
.intent, .entity LocaleResources.find through OVOSSkill._locate_lang_file.
.blacklist LocaleResources.load_blacklist: the intent's own file, a <slot>.blacklist for each declared slot, and <entity>.blacklist for each entity file (OVOS-INTENT-2 §4.3). The registration payload is unchanged.
skill.json The language folder comes from find_lang_dir, so locale/en-us/ serves en-US.
.rx load_regex_files in _LegacyResourcesMixin, self-contained and deprecated. register_regex passes the pattern to register_adapt_regex, which prefixes the group names once.
Language directory OVOSAbstractApplication.get_language_dir calls ovos_spec_tools.find_lang_dir.

A malformed or duplicate resource is logged once and skipped where it is read: register_intent_file, register_entity_file, render_dialog, voc_list, the blacklists and the vocabularies. It no longer aborts skill start or a handler.

load_lang returns one object per resource root and language. Each object holds a snapshot of the skill's locale tree, so a caller that passes a language, such as ovos-good-manners-pipeline-plugin for each language, adds one snapshot per language. The first load for a language also registers every shipped .entity file for it, once per resource root and language, and skills.auto_register_entity_files still turns that off.

OVOSSkill looks up resources in self.lang, and render_dialog takes a lang argument. UniversalSkill overrides render_dialog, voc_list, register_vocabulary and register_regex to default to its internal language. The dead UniversalSkill._load_lang override is removed.

.required keywords load through LocaleResources.load_required_keywords, beside the .blacklist, and travel on the registration as required_keywords.

Other changes in this branch

These go beyond the refactor.

Change Reason
@homescreen_app and @resting_screen_handler warn when applied; register_homescreen_app and register_resting_screen are deprecated and live in _LegacyResourcesMixin. The homescreen concept is marked for removal and has no replacement.
UniversalSkill.speak calls translate_utterance with the target and source languages the right way round. The arguments were swapped, so spoken output was translated away from the user's language.
UniversalSkill skips __tags__ translation when the message has no __tags__. Engines other than Adapt do not set it.
word_connectors.json and euphony.json load through iter_locale_dirs and closest_lang. CoreResources is deprecated.
The canonical keyword of a .voc line is the alphabetically first alternative. It follows from LocaleResources.vocabulary_keywords. A skill registers only its own .voc files; the workshop's cancel.voc serves voc_match in get_response and is not an Adapt keyword.

User overrides

A user override is read from <xdg data>/resources/<skill_id>/<lang>/, the path dev reads. OVOS-INTENT-2 §2.1 names <skill_id>/locale/<lang>/; the spec path is not read and this branch does not change that. A test shows that a user dialog at the supported path wins over the skill's own.

Dependencies

ovos-spec-tools>=1.16.0a1, the first release with LocaleResources.load_required_keywords. The floors of ovos-utils and ovos_bus_client on dev already provide the rest.

Tests

pytest test passes: 879 passed, 1 skipped (dev: 827 passed, 1 skipped, same venv). test/unittests/test_required_file.py from dev passes, and fails 2 tests when the loader returns no keywords. test/unittests/test_stable_skill_shapes.py has 42 regression tests built from file shapes found in skills: duplicate intents and entities in nested folders and in intent/ against intents/, which are reported and skipped, a dialog that mixes lines with and without {time}, the speedtest, randomness and ip dialogs, empty .dialog and .voc files, a lowercase locale/en-us/skill.json, the load_lang(...) calls of ovos-good-manners-pipeline-plugin, a single prefix on register_regex groups, one WARNING per malformed file, a user override, and the DialogNotFound error and its message, the handler error path, mixed-slot dialogs, and a source-level check that only the shims touch the legacy path. They cover the literal-prompt fallback (a dialog of another language, or one that cannot render, raises) and the first-run intro. The rest guard behaviour that already held.

🤖 Generated with Claude Code

@coderabbitai

coderabbitai Bot commented May 25, 2026 •

Copy link
Copy Markdown
Contributor
Walkthrough
📝 Walkthrough

Walkthrough

This PR migrates resource lookup/rendering to ovos-spec-tools LocaleResources, deprecates the legacy ovos_workshop.resource_files APIs (adding runtime deprecation warnings), adds a legacy compatibility mixin for OVOSSkill, updates language handling across skills, and adds extensive unit and end-to-end tests and locale fixtures.

Changes

Resource handling modernization and deprecation

Layer / File(s) Summary
Dependencies and core infrastructure
.gitignore, pyproject.toml, ovos_workshop/app.py
Added ovos-spec-tools to dependencies, bumped ovos-utils minimum, updated OVOSAbstractApplication.get_language_dir to use ovos_spec_tools.find_lang_dir/standardize_lang, and ignored test temp config directory.
Deprecation warnings in public decorator APIs
ovos_workshop/decorators/__init__.py
Added DeprecationWarning emissions to resting_screen_handler and homescreen_app decorator factories via warnings.warn() and updated docstrings documenting deprecation.
Resource files module refactoring and deprecation
ovos_workshop/resource_files.py
Marked module and legacy resource classes deprecated with _deprecated_public; switched DialogFile/IntentFile/VocabularyFile rendering and expansion to use ovos_spec_tools (expand/render), added DialogFile._load_raw(), updated language matching to ovos_spec_tools.lang_distance, and rewired SkillResources.dialog_renderer to lazily build Mustache templates while emitting deprecation warnings.
Legacy resource compatibility mixin
ovos_workshop/skills/_legacy_resources.py
Added _LegacyResourcesMixin exposing deprecated shims (resources, dialog_renderer, find_resource(), voc_match_cache, runtime_requirements, network_requirements, load_dialog_files) that emit DeprecationWarning and route through cached legacy SkillResources.
Core OVOSSkill modernization
ovos_workshop/skills/ovos.py
Refactored OVOSSkill to use ovos_spec_tools.LocaleResources (cached per root) for dialog/voc/intent/entity lookups and rendering, added render_dialog(name, data), separated query vs resource language (_resource_lang/_locale_resources), updated .voc/.rx loading/registration to use LocaleResources and spectools helpers, and wrapped homescreen/resting registrations with deprecation warnings.
UniversalSkill, converse, and util updates
ovos_workshop/skills/auto_translatable.py, ovos_workshop/skills/converse.py, ovos_workshop/skills/util.py
Added UniversalSkill._resource_lang returning internal_language, fixed translation call argument mapping in speak(), standardized converse language matching and intent file resolution with ovos_spec_tools, and replaced CoreResources JSON loading with a _load_workshop_json() helper that selects closest locale JSON.
Test locale fixtures and resources
test/end2end/locale/*, test/end2end/universal_locale/locale/*
Added and updated dialog, intent, voc, entity, and rx fixture files across en-US and pt-PT locales to support new and existing E2E tests.
End-to-end integration tests
test/end2end/test_*.py
Added multiple E2E tests verifying entity and intent file registration, regex intent matching, speak_dialog rendering with slot and voc expansion, voc_match behavior, and UniversalSkill resource-language decoupling across sessions.
Unit test updates and legacy resource coverage
test/unittests/skills/test_*.py
Updated unit tests to assert LocaleResources usage, added test_legacy_resources.py (broad legacy-shim coverage), test_legacy_skill_omnibus.py (omnibus legacy checks), and test_regex_loading.py (pin .rx behavior and deprecation timing).

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Pre-merge checks: Passed 4
Check name Status Explanation
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Title check Passed The title clearly and concisely describes the primary change: routing OVOSSkill resource loading through ovos-spec-tools while matching the refactor scope.
Description Check Passed Check skipped - CodeRabbit’s high-level summary is enabled.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/drop-resource-files-usage
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented May 25, 2026 •

Copy link
Copy Markdown
Contributor

Greetings, human! The automated checks are complete. 👾

I've aggregated the results of the automated checks for this PR below.

📋 Repo Health

I've checked the repo's social skills (aka issue response time). 🗣️

✅ All required files present.

Latest Version: 9.10.1a1

✅ ovos_workshop/version.py — Version file
✅ README.md — README
✅ LICENSE — License file
✅ pyproject.toml — pyproject.toml
⚠️ setup.py — setup.py
✅ CHANGELOG.md — Changelog
✅ ovos_workshop/version.py has valid version block markers

⚖️ License Check

I've verified the license compliance for your changes. ✅

✅ No license violations found.

Policy: Apache 2.0 (universal donor). StrongCopyleft / NetworkCopyleft / WeakCopyleft / Other / Error categories fail. MPL allowed.

🔍 Lint

Everything looks good so far! ✅

❌ ruff: issues found in 117 file(s) — see job log
✅ actionlint: 11 workflow file(s), no finding

🔒 Security (pip-audit)

Scanning for any potential privilege escalations. 🪜

⚠️ 13 known vulnerabilities in 1 package (72 packages scanned). This job warns and does not fail; update the affected packages.

Package Version ID Description Fix
filelock 3.32.7 CVE-2026-112940 ReadWriteLock and AsyncReadWriteLock (the SQLite-backed reader/writer lock) hand SQLite a name they do not contro... 4.2.1
filelock 3.32.7 CVE-2026-112913 On Windows, ReadWriteLock and AsyncReadWriteLock (the SQLite-backed reader/writer lock) connect SQLite to the loc... 4.2.1
filelock 3.32.7 CVE-2026-112907 On Windows the soft locks (SoftFileLock, MarkerSoftFileLock, SoftFileLease, their async forms, and a FileLock... 4.2.1
filelock 3.32.7 CVE-2026-112938 StrictSoftFileLock and AsyncStrictSoftFileLock check <path>.filelock (the coordination directory that holds the... 4.2.1
filelock 3.32.7 CVE-2026-112936 ### Summary ReadWriteLock and AsyncReadWriteLock resolve the lock path with pathlib.Path.resolve() and pass it ... 4.0.9
filelock 3.32.7 CVE-2026-112910 filelock on Windows ignores the mode argument. FileLock(path, mode=0o600) creates the same file as `FileLock(path... 4.2.1
filelock 3.32.7 CVE-2026-112909 UnixFileLock, the backend of FileLock and AsyncFileLock on Unix, empties whatever regular file it finds at the ... 4.2.1
filelock 3.32.7 CVE-2026-112911 StrictSoftFileLock, AsyncStrictSoftFileLock, SoftReadWriteLock and AsyncSoftReadWriteLock keep their whole ow... 4.2.1
filelock 3.32.7 CVE-2026-112939 SoftFileLock, MarkerSoftFileLock, SoftFileLease, their async variants and a FileLock that has fallen back to ... 4.2.1
filelock 3.32.7 CVE-2026-112915 ReadWriteLock and AsyncReadWriteLock (the SQLite-backed reader/writer lock) run PRAGMA journal_mode=MEMORY on a... 4.2.1
filelock 3.32.7 CVE-2026-112935 The stale-lock break shared by SoftFileLock, MarkerSoftFileLock and SoftFileLease (and their async variants, an... 4.2.1
filelock 3.32.7 CVE-2026-112912 raise_on_not_writable_file() is the check that SoftFileLock and the locks sharing its acquire path run before eve... 4.2.1
filelock 3.32.7 CVE-2026-112908 StrictSoftFileLock and AsyncStrictSoftFileLock apply the caller's mode to the sentinel (the lock file at `<path... 4.2.1

🔨 Build Tests

Ensuring the scaffolding is removed and the build is clean. 🧹

✅ All versions pass

Python Build Install Tests pytest
3.10 ✅ ✅ ✅ 855 passed, 2876 warnings, 46 subtests passed in 89.61s (0:01:29)
3.11 ✅ ✅ ✅ 855 passed, 2878 warnings, 46 subtests passed in 80.92s (0:01:20)
3.12 ✅ ✅ ✅ 855 passed, 2878 warnings, 46 subtests passed in 85.16s (0:01:25)
3.13 ✅ ✅ ✅ 855 passed, 2877 warnings, 46 subtests passed in 88.14s (0:01:28)
3.14 ✅ ✅ ✅ 855 passed, 2877 warnings, 46 subtests passed in 82.80s (0:01:22)

📡 Channel Compat — testing

The data is in, and it's looking interesting! 🧐

🚧 Channel unresolvable with this checkout — the repo's dependency floors exceed what the channel pins (fleet finding; see the install log).

Constraints: https://raw.githubusercontent.com/OpenVoiceOS/OpenVoiceOS/main/constraints-testing.txt
Resolved stack: channel-compat-testing artifact (constraints + pip freeze).

📡 Channel Compat — stable

Another day, another successful automated run. 🌅

🚧 Channel unresolvable with this checkout — the repo's dependency floors exceed what the channel pins (fleet finding; see the install log).

Constraints: https://raw.githubusercontent.com/OpenVoiceOS/OpenVoiceOS/main/constraints-stable.txt
Resolved stack: channel-compat-stable artifact (constraints + pip freeze).


Keeping the OVOS ecosystem thriving 🌿

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
ovos_workshop/skills/ovos.py (1)

739-763: ⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Don't mask the original startup failure when status is still unset.

If _init_settings() or bind() throws before self.status is assigned, the except path raises a second AttributeError here and hides the real cause. That's exactly what the current CI failures are showing.

💡 Suggested fix
         except Exception as e:
-            self.status.set_error(str(e))
+            if hasattr(self, "status"):
+                self.status.set_error(str(e))
+            else:
+                LOG.exception("Skill startup failed before ProcessStatus initialization")
             # If an exception occurs, attempt to clean up the skill
             try:
                 self.default_shutdown()
🤖 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` around lines 739 - 763, The except block can
raise AttributeError when self.status was never set (e.g., if _init_settings()
or bind() failed); update the exception handler to avoid masking the original
error by guarding access to self.status and default_shutdown: check
hasattr(self, "status") and self.status is not None before calling
self.status.set_error(str(e)) or self.status.* methods, and protect
default_shutdown call similarly (or call it only if it exists), then re-raise
the original exception; reference symbols: self.status, _init_settings, bind,
default_shutdown, self.status.set_error.
🧹 Nitpick comments (1)
ovos_workshop/skills/util.py (1)

30-50: ⚡ Quick win

Cache locale JSON resolution to avoid repeated filesystem I/O.

_load_workshop_json currently rescans locale dirs and re-reads JSON on each call; join_word_list can hit this path frequently.

💡 Suggested refactor
+from functools import lru_cache
 import json
 import os
 from os.path import dirname, isdir, isfile, join
@@
+@lru_cache(maxsize=1)
+def _available_locale_dirs() -> Dict[str, str]:
+    return {lang: str(path) for lang, path in iter_locale_dirs(_LOCALE_ROOT)}
+
+@lru_cache(maxsize=64)
 def _load_workshop_json(name: str, lang: str) -> Optional[Dict]:
@@
-    available = dict(iter_locale_dirs(_LOCALE_ROOT))  # {lang_norm: Path}
+    available = _available_locale_dirs()  # {lang_norm: path}
@@
-    path = join(str(available[best]), f"{name}.json")
+    path = join(available[best], f"{name}.json")
🤖 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/util.py` around lines 30 - 50, The function
_load_workshop_json repeatedly rescans locale dirs and reloads JSON on every
call (used frequently by join_word_list); cache results to avoid filesystem I/O
by memoizing the available locale mapping and parsed JSON per (name, lang) pair.
Specifically, keep a module-level cache (or use functools.lru_cache) for
iter_locale_dirs(_LOCALE_ROOT) output (used in available =
dict(iter_locale_dirs(...))) and for loaded JSON results of json.load(path)
keyed by (name, best) so subsequent calls return the cached dict instead of
re-reading the file; ensure cache is invalidated/refreshable if needed (e.g.,
provide a clear/reset helper) and preserve current behavior when files are
missing.
🤖 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 `@ovos_workshop/app.py`:
- Line 4: The module removed the join import but settings_path still uses join,
causing runtime/lint failures; re-add join to the import list so settings_path
can use it—specifically update the top-level import that currently reads "from
ovos_spec_tools import find_lang_dir, standardize_lang" to also import join so
the function/settings_path and any uses of join resolve correctly.

In `@ovos_workshop/skills/_legacy_resources.py`:
- Around line 59-69: The _legacy_skill_resources method currently snapshots
self.lang into SkillResources and never updates; change it to use
self._resource_lang when constructing SkillResources and add logic to
invalidate/rebuild the cached self._skill_resources_compat whenever
self._resource_lang changes (store the last used lang or compare current
self._resource_lang to the lang on the cached SkillResources), so callers like
resources, dialog_renderer and find_resource get a SkillResources instance
matching the current _resource_lang; update the same pattern in the other
shim(s) referenced (lines 127-147) to ensure cache is refreshed on language
change.

In `@ovos_workshop/skills/ovos.py`:
- Around line 744-748: Startup now calls private implementations
(_load_data_files, _register_skill_json, _register_decorated,
_register_app_launcher, _register_resting_screen) which bypasses subclass
overrides; change calls so public methods (load_data_files,
register_skill_json/register_decorated if present, register_homescreen_app,
register_resting_screen) are invoked first and fall back to their underscored
counterparts if the public method is not overridden. Update _startup,
_register_app_launcher, and any other locations (e.g., around lines 791-793 and
2403-2439) to call the public shim methods like load_data_files and
register_homescreen_app instead of directly calling
_load_data_files/_register_app_launcher, preserving subclass override
compatibility while keeping private implementations as fallbacks.

In `@ovos_workshop/skills/util.py`:
- Around line 49-50: The locale JSON loader opens files using the system default
encoding; change the open call in ovos_workshop/skills/util.py to explicitly use
UTF-8 by passing encoding='utf-8' (e.g., in the with open(path, ...) statement
that wraps json.load), so that parsing localized content works correctly across
environments.

In `@test/end2end/test_regex_skill.py`:
- Around line 72-75: The on_speak handler currently sets speak_event for every
speak message which can unblock the test prematurely; change on_speak (and the
test wiring around seen_speaks/speak_event) to only set speak_event when the
utterance matches the expected output (e.g., compare
msg.data.get("utterance","") against the expected string/regex or check
substring) and still append seen_speaks as needed, so the wait only wakes for
the regex handler's emission.

In `@test/unittests/skills/test_legacy_skill_omnibus.py`:
- Around line 184-185: The tearDownClass currently swallows all exceptions from
cls.skill.default_shutdown() with "except Exception: pass"; change this to catch
Exception as e and handle it instead of silencing—either re-raise after logging
or call a test logger/assertFail to surface the error; specifically update the
tearDownClass method to wrap cls.skill.default_shutdown() in try/except
Exception as e and call something like logging.exception or raise to ensure
failures are reported rather than ignored.

---

Outside diff comments:
In `@ovos_workshop/skills/ovos.py`:
- Around line 739-763: The except block can raise AttributeError when
self.status was never set (e.g., if _init_settings() or bind() failed); update
the exception handler to avoid masking the original error by guarding access to
self.status and default_shutdown: check hasattr(self, "status") and self.status
is not None before calling self.status.set_error(str(e)) or self.status.*
methods, and protect default_shutdown call similarly (or call it only if it
exists), then re-raise the original exception; reference symbols: self.status,
_init_settings, bind, default_shutdown, self.status.set_error.

---

Nitpick comments:
In `@ovos_workshop/skills/util.py`:
- Around line 30-50: The function _load_workshop_json repeatedly rescans locale
dirs and reloads JSON on every call (used frequently by join_word_list); cache
results to avoid filesystem I/O by memoizing the available locale mapping and
parsed JSON per (name, lang) pair. Specifically, keep a module-level cache (or
use functools.lru_cache) for iter_locale_dirs(_LOCALE_ROOT) output (used in
available = dict(iter_locale_dirs(...))) and for loaded JSON results of
json.load(path) keyed by (name, best) so subsequent calls return the cached dict
instead of re-reading the file; ensure cache is invalidated/refreshable if
needed (e.g., provide a clear/reset helper) and preserve current behavior when
files are missing.
🪄 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: cbb8676b-dfdc-4473-8eae-926b41640555

📥 Commits

Reviewing files that changed from the base of the PR and between 779e54b and 4e53715.

📒 Files selected for processing (32)
  • .gitignore
  • ovos_workshop/app.py
  • ovos_workshop/decorators/__init__.py
  • ovos_workshop/resource_files.py
  • ovos_workshop/skills/_legacy_resources.py
  • ovos_workshop/skills/auto_translatable.py
  • ovos_workshop/skills/converse.py
  • ovos_workshop/skills/ovos.py
  • ovos_workshop/skills/util.py
  • pyproject.toml
  • test/end2end/__init__.py
  • test/end2end/locale/en-US/drink.entity
  • test/end2end/locale/en-US/greet.dialog
  • test/end2end/locale/en-US/play.rx
  • test/end2end/locale/en-US/salutation.voc
  • test/end2end/locale/en-US/wave.intent
  • test/end2end/locale/en-US/yes.voc
  • test/end2end/test_padacioso_entity_file.py
  • test/end2end/test_padacioso_intent_file.py
  • test/end2end/test_regex_skill.py
  • test/end2end/test_speak_dialog.py
  • test/end2end/test_universal_skill.py
  • test/end2end/test_voc_match.py
  • test/end2end/universal_locale/locale/en-US/affirmative.voc
  • test/end2end/universal_locale/locale/en-US/echo.dialog
  • test/end2end/universal_locale/locale/en-US/fact.intent
  • test/end2end/universal_locale/locale/pt-PT/fact.intent
  • test/unittests/skills/test_base.py
  • test/unittests/skills/test_legacy_resources.py
  • test/unittests/skills/test_legacy_skill_omnibus.py
  • test/unittests/skills/test_ovos.py
  • test/unittests/skills/test_regex_loading.py

Comment thread ovos_workshop/app.py Resolved
Comment thread ovos_workshop/skills/ovos.py Resolved
Comment thread ovos_workshop/skills/util.py Resolved
Comment on lines +72 to +75
def on_speak(msg):
seen_speaks.append(msg.data.get("utterance", ""))
speak_event.set()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Gate the wait event on the expected utterance to prevent flakiness.

on_speak sets the event for every speak message. If unrelated speech arrives first, the wait can unblock before the regex handler emits its output.

Suggested fix
         def on_speak(msg):
-            seen_speaks.append(msg.data.get("utterance", ""))
-            speak_event.set()
+            utt = msg.data.get("utterance", "")
+            seen_speaks.append(utt)
+            if "playing music" in utt:
+                speak_event.set()
🤖 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_regex_skill.py` around lines 72 - 75, The on_speak handler
currently sets speak_event for every speak message which can unblock the test
prematurely; change on_speak (and the test wiring around
seen_speaks/speak_event) to only set speak_event when the utterance matches the
expected output (e.g., compare msg.data.get("utterance","") against the expected
string/regex or check substring) and still append seen_speaks as needed, so the
wait only wakes for the regex handler's emission.

Comment on lines +184 to +185
except Exception:
pass

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify whether silent broad-exception swallowing exists in this test module
rg -nP 'except\s+Exception:\s*\n\s*pass' test/unittests/skills/test_legacy_skill_omnibus.py

Repository: OpenVoiceOS/ovos-workshop

Length of output: 51


🏁 Script executed:

#!/bin/bash
set -euo pipefail

file="test/unittests/skills/test_legacy_skill_omnibus.py"
# Print the claimed area with line numbers
nl -ba "$file" | sed -n '165,205p'

Repository: OpenVoiceOS/ovos-workshop

Length of output: 112


🏁 Script executed:

#!/bin/bash
set -euo pipefail

file="test/unittests/skills/test_legacy_skill_omnibus.py"
awk 'NR>=165 && NR<=205 { printf "%d:%s\n", NR, $0 }' "$file"

Repository: OpenVoiceOS/ovos-workshop

Length of output: 2046


🏁 Script executed:

#!/bin/bash
set -euo pipefail
file="test/unittests/skills/test_legacy_skill_omnibus.py"
# Show the import section and any warnings usage
rg -n '^\s*(import|from)\s+warnings|warnings\.' "$file" || true
head -n 60 "$file"

Repository: OpenVoiceOS/ovos-workshop

Length of output: 2616


Avoid silently swallowing teardown exceptions in tearDownClass.

In test/unittests/skills/test_legacy_skill_omnibus.py lines 184–185, except Exception: pass hides failures from cls.skill.default_shutdown().

Proposed fix
-        except Exception:
-            pass
+        except Exception as e:
+            warnings.warn(
+                f"default_shutdown failed during teardown: {e!r}",
+                RuntimeWarning
+            )
🧰 Tools
🪛 Ruff (0.15.14)

[error] 184-185: try-except-pass detected, consider logging the exception

(S110)


[warning] 184-184: Do not catch blind exception: Exception

(BLE001)

🤖 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 184 - 185,
The tearDownClass currently swallows all exceptions from
cls.skill.default_shutdown() with "except Exception: pass"; change this to catch
Exception as e and handle it instead of silencing—either re-raise after logging
or call a test logger/assertFail to surface the error; specifically update the
tearDownClass method to wrap cls.skill.default_shutdown() in try/except
Exception as e and call something like logging.exception or raise to ensure
failures are reported rather than ignored.

@JarbasAl
JarbasAl marked this pull request as draft June 23, 2026 16:22
@JarbasAl
JarbasAl marked this pull request as ready for review July 3, 2026 15:03
@JarbasAl
JarbasAl marked this pull request as draft July 3, 2026 19:51
@JarbasAl
JarbasAl force-pushed the feat/drop-resource-files-usage branch from e842cb2 to 9f7efc7 Compare July 3, 2026 23:40
@JarbasAl
JarbasAl force-pushed the feat/drop-resource-files-usage branch 2 times, most recently from da27ca2 to 9f4883f Compare July 26, 2026 12:45
@JarbasAl
JarbasAl force-pushed the feat/drop-resource-files-usage branch 2 times, most recently from 83d8792 to 1022f3c Compare October 12, 2026 21:12
@JarbasAl JarbasAl changed the title refactor: route OVOSSkill resource loading through ovos-spec-tools (back-compat) refactor: route OVOSSkill resource loading through ovos-spec-tools Oct 12, 2026
@JarbasAl
JarbasAl force-pushed the feat/drop-resource-files-usage branch 2 times, most recently from 84b281b to a0ab65c Compare October 13, 2026 01:01
OVOSSkill and the rest of ovos_workshop read skill resources through
ovos_spec_tools.LocaleResources and use nothing else. Dialogs render with
ovos_spec_tools.render, vocabularies and blacklists load from LocaleResources,
and .intent and .entity files resolve with LocaleResources.find. The first load
for a language also registers every shipped .entity file for it, once per
resource root and language.

A missing dialog raises DialogNotFound, a subclass of FileNotFoundError. The
message names the dialog, the language and the directories searched.
OVOS-INTENT-2 section 2: "a missing `.dialog` leaves the handler with nothing
to speak for that response in that language". A dialog the specification
rejects as a whole renders the phrases that are valid for the given data and
logs one warning; when no phrase renders it raises DialogNotFound with the
file and the reason. get_response, ask_yesno and ask_selection keep their
documented form of a dialog resource or a string.

A resource that is empty or malformed is reported once and skipped where it is
read, so it no longer aborts skill start or a handler. When two files share a
base name, the shallowest file is used and a tie goes to the last path in
sorted order; one warning names both paths. The legacy dialog/, vocab/ and
regex/ layouts are not supported; OVOS-INTENT-2 defines locale/<lang>/ only.

ovos_workshop.resource_files stays importable for outside callers, and its
public functions and classes emit a DeprecationWarning. load_lang returns a
LocaleResources that also answers the SkillResources methods, with a
deprecation warning. The other legacy OVOSSkill members (resources,
dialog_renderer, find_resource, load_dialog_files, voc_match_cache,
runtime_requirements, network_requirements) live on _LegacyResourcesMixin. The
shims raise DialogNotFound for a missing dialog too.

skill.json is found through find_lang_dir, so a lowercase language folder
works. register_regex leaves the group-name prefix to register_adapt_regex.
OVOSAbstractApplication.get_language_dir resolves through
ovos_spec_tools.find_lang_dir. UniversalSkill sets _resource_lang to its
internal language.

Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
@JarbasAl
JarbasAl force-pushed the feat/drop-resource-files-usage branch from a0ab65c to 1f9c26b Compare October 13, 2026 02:47
The .required keywords from dev load through
LocaleResources.load_required_keywords, like the blacklist beside them,
so the skill still builds no legacy resource object. The
ovos-spec-tools floor moves to 1.16.0a1, the first release with that
method.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
JarbasAl and others added 4 commits October 29, 2026 12:35
Take #737: the workshop's own dialogs in every locale, and skill.error
renamed to skill_error. The error handler renders skill_error. The
test for a language with no error dialog uses zu-ZA, because de-DE
ships one.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
LocaleResources.vocabulary_keywords walks the core locale too, so every
skill registered the workshop's cancel.voc as an adapt keyword. Nothing
requires that keyword: get_response reads cancel.voc through voc_match.
Skip a .voc that resolves into the workshop locale.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Remove _locale_resources, which only returned load_lang(). Inline
_ships_dialog and _pick_duplicate into their one caller.

Move the deprecated methods into _LegacyResourcesMixin:
load_regex_files, register_homescreen_app, register_resting_screen and
the @homescreen_app scan.

A resource that shares its base name with another file in one language
tree is malformed (OVOS-INTENT-2 §2). It is reported once and skipped.
The workshop no longer picks the shallowest file.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…self.lang

OVOSSkill looks up dialogs, vocabulary and adapt registrations in
self.lang. render_dialog takes a lang argument, like voc_list,
register_vocabulary and register_regex. UniversalSkill overrides these
four methods to default to internal_language, so the base class needs
no language hook.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant