feat: yesno/selection agent plugins#390
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReplaces internal yes/no and option-matching logic with configurable plugin engines for OVOSSkill, adds engine-loading/caching, updates ask_yesno/ask_selection to delegate to plugins with fallbacks, adds docs, updates dependencies, and introduces unit tests covering engine resolution and behaviors. Changes
Sequence Diagram(s)sequenceDiagram
participant Skill as OVOSSkill
participant Cache as Engine Cache
participant PluginMgr as Plugin Manager
participant Engine as YesNo Engine
participant Solver as Heuristic Solver
rect rgba(200,150,100,0.5)
Note over Skill,Solver: ask_yesno() flow
Skill->>Cache: check cached yesno engine
Cache-->>Skill: cache miss / hit
alt cache miss
Skill->>PluginMgr: load yesno plugin (settings → config_core → default)
PluginMgr-->>Engine: return engine instance / raise
Engine-->>Cache: store engine
end
Skill->>Engine: yes_or_no(question=prompt, response=resp, lang=lang)
Engine-->>Skill: True / False / None
alt engine returned True/False
Skill-->>Skill: map True->"yes", False->"no"
else engine returned None or unavailable
Skill->>Solver: match_yes_or_no(response=resp, lang=lang)
Solver-->>Skill: match / raw resp / None
end
end
sequenceDiagram
participant Skill as OVOSSkill
participant Cache as Engine Cache
participant PluginMgr as Plugin Manager
participant Engine as OptionMatcher Engine
rect rgba(100,150,200,0.5)
Note over Skill,Engine: ask_selection() flow
Skill->>Cache: check cached selection engine
Cache-->>Skill: cache miss / hit
alt cache miss
Skill->>PluginMgr: load option-matcher plugin (settings → config_core → default)
PluginMgr-->>Engine: return engine instance / raise
Engine-->>Cache: store engine
end
Skill->>Engine: match_option(utterance=resp, options=options, lang=lang)
Engine-->>Skill: matched option / None
alt engine returns option
Skill-->>Skill: return matched option
else engine error or None
Skill-->>Skill: log and return None
end
end
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
Contributor
Just the facts, ma'am. Here's your report. 👮♂️I've aggregated the results of the automated checks for this PR below. 📋 Repo HealthI've performed a holistic audit of your project's soul. 🧘 ✅ All required files present. Latest Version: ✅ 🔍 LintChecking the boxes and crossing the T's. 🖋️ ❌ ruff: issues found — see job log 🔨 Build TestsEnsuring the gears are properly lubricated. 💧
❌ 3.10: unknown 🔒 Security (pip-audit)Checking for any insecure data transmissions. 📡 ✅ No known vulnerabilities found (67 packages scanned). ⚖️ License CheckChecking for any potential legal hurdles. 🚧 ✅ No license violations found (48 packages). License distribution: 12× MIT License, 8× 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 — 48 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. Your friendly neighborhood bot 🕷️ |
![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
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)
2007-2057:⚠️ Potential issue | 🟡 MinorThe
min_confparameter is now unused.The
min_confparameter (line 2008) is no longer used in the method body since the legacymatch_onefallback was removed. The parameter signature should either be deprecated or documented as passed to the engine via config.Additionally, when the engine raises an exception (lines 2052-2056), the method returns
Nonewith no fallback matching. This differs from the documentation indocs/skill-interaction.mdline 345 which statesask_yesnofalls back toYesNoSolveron plugin failure, butask_selectiondoes not have equivalent fallback behavior—verify this asymmetry is intentional.Consider documenting or deprecating min_conf
Either remove the parameter with a deprecation warning:
def ask_selection(self, options: List[str], dialog: str = '', data: Optional[dict] = None, min_conf: float = 0.65, numeric: bool = False, num_retries: int = -1): + # Note: min_conf is deprecated and ignored; configure via plugin settingsOr pass it to the engine:
try: - resp = engine.match_option(utterance=resp, options=options, lang=self.lang) + resp = engine.match_option(utterance=resp, options=options, lang=self.lang, min_conf=min_conf)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovos_workshop/skills/ovos.py` around lines 2007 - 2057, The ask_selection method currently ignores the min_conf parameter and has no fallback when engine.match_option fails; update ask_selection to either (A) deprecate/remove min_conf (emit a DeprecationWarning in ask_selection and update docstring), or (B) pass min_conf to the selection engine by calling engine.match_option(utterance=resp, options=options, lang=self.lang, min_conf=min_conf) so engines can use it; additionally add a safe fallback path in ask_selection in the except block (e.g., call a legacy matcher or a default solver similar to YesNoSolver) so that when _get_selection_engine() exists but engine.match_option raises, the method attempts fallback matching instead of returning None, and ensure logging still records the exception (LOG.error) before falling back.
🧹 Nitpick comments (4)
test/unittests/test_skill_interaction.py (2)
78-112: Missing test for selection engine caching.
TestGetYesnoEngineincludestest_engine_cached_across_calls(lines 69-75), butTestGetSelectionEnginelacks an equivalent test. Consider adding one for consistency:Add caching test for selection engine
def test_engine_cached_across_calls(self): skill = _make_skill() # Uses default fuzzy plugin mock_cls = MagicMock() with patch("ovos_plugin_manager.agents.load_option_matcher_plugin", return_value=mock_cls) as mock_load: skill._get_selection_engine() skill._get_selection_engine() mock_load.assert_called_once()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_skill_interaction.py` around lines 78 - 112, Add a caching test to TestGetSelectionEngine to mirror TestGetYesnoEngine: create a new test method (e.g. test_engine_cached_across_calls) that calls skill._get_selection_engine() twice while patching ovos_plugin_manager.agents.load_option_matcher_plugin to return a MagicMock and then assert the plugin load was called only once; reference TestGetSelectionEngine, _get_selection_engine, and load_option_matcher_plugin when locating where to add the test.
218-224: Clarify test intent: no fallback when engine is None.This test (
test_no_engine_no_response_returns_none) patches_get_selection_engineto returnNone, but in production_get_selection_enginewill never returnNoneunless the default fuzzy plugin fails to load. Consider adding a test for when the engine exists but returnsNonefrommatch_option, which is the more common "no match" scenario.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/unittests/test_skill_interaction.py` around lines 218 - 224, The test test_no_engine_no_response_returns_none incorrectly simulates a missing engine by patching _get_selection_engine to return None; instead, add or replace a test that simulates a present engine that yields no match by patching _get_selection_engine to return a mock engine whose match_option method returns None, then call ask_selection(options, numeric=True) and assert it returns None; reference the test name, _get_selection_engine, match_option, and ask_selection so the change targets the selection flow where the engine exists but finds no match.docs/skill-interaction.md (1)
46-49: Add language specifiers to fenced code blocks.Several code blocks for dialog files lack language specifiers. While these are plain text, adding a specifier improves rendering and satisfies linters.
Add language specifiers
-``` +```text Are you sure you want to delete this?Apply similarly to lines 58, 138, and 266.
Also applies to: 58-60, 138-141, 266-268
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/skill-interaction.md` around lines 46 - 49, Several fenced code blocks containing dialog snippets (e.g., the blocks with text "Are you sure you want to delete this?", "Do you really want to delete it?", and the other plain-text dialog blocks around the same sections) lack language specifiers; update each triple-backtick fence to include a language tag (use "text") so the blocks become ```text ... ```; apply this change to the code fences containing the shown lines and the similar blocks at the other locations referenced in the comment.ovos_workshop/skills/ovos.py (1)
1964-1982: Default plugin always triggers load attempt.Unlike
_get_yesno_engine, this method defaults to"ovos-option-matcher-fuzzy-plugin"when no config is set (line 1972). This means every call will attempt to load a plugin even if the user hasn't configured one. If the default plugin isn't installed, this will log an error on everyask_selectioncall.Consider whether this is intentional—currently
pyproject.tomladds the fuzzy plugin as a dependency, so it should always be available. However, if someone removes it, the error will be noisy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovos_workshop/skills/ovos.py` around lines 1964 - 1982, The method _get_selection_engine currently defaults plugin_name to "ovos-option-matcher-fuzzy-plugin" causing a load attempt on every call; change it to mirror _get_yesno_engine by not defaulting to a plugin: derive plugin_name only from self.settings and self.config_core (no hardcoded default), compute cache_key the same, and only call load_option_matcher_plugin(plugin_name) and instantiate when plugin_name is truthy; if no plugin_name set, set the cache attribute to None so future calls are cached and avoid repeated error logs from load_option_matcher_plugin.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/skill-interaction.md`:
- Around line 104-114: The docs claim the min_conf parameter is applied but the
implementation no longer uses it directly; update the documentation for the
options parameter table to state that min_conf is currently unused in the
high-level API (or is forwarded only to the OptionMatcherEngine configuration if
applicable) or mark min_conf as deprecated. Specifically, change the `min_conf`
row to indicate its current behavior (e.g., "currently unused in the high-level
selection flow; only used when configuring OptionMatcherEngine" or "deprecated")
and mention the OptionMatcherEngine symbol so readers can find where to
set/override this value.
- Around line 340-349: The doc claims ask_yesno falls back to YesNoSolver if the
plugin raises, but the code calls engine.yes_or_no() directly; wrap the
engine.yes_or_no() call inside a try/except in the ask_yesno implementation so
any exception from the engine is caught, log the exception, and then invoke the
fallback YesNoSolver path (same return behavior as when plugin fails to load) to
match the documentation; ensure you reference engine.yes_or_no(), ask_yesno, and
YesNoSolver when making the change so the fallback is triggered on runtime
errors.
In `@test/unittests/test_skill_interaction.py`:
- Line 1: Update the copyright header string from "2024" to "2026" in the file
(the top-line copyright comment), ensuring the header now reads "Copyright 2026
Mycroft AI Inc." so the year is current; keep formatting identical aside from
the year change.
---
Outside diff comments:
In `@ovos_workshop/skills/ovos.py`:
- Around line 2007-2057: The ask_selection method currently ignores the min_conf
parameter and has no fallback when engine.match_option fails; update
ask_selection to either (A) deprecate/remove min_conf (emit a DeprecationWarning
in ask_selection and update docstring), or (B) pass min_conf to the selection
engine by calling engine.match_option(utterance=resp, options=options,
lang=self.lang, min_conf=min_conf) so engines can use it; additionally add a
safe fallback path in ask_selection in the except block (e.g., call a legacy
matcher or a default solver similar to YesNoSolver) so that when
_get_selection_engine() exists but engine.match_option raises, the method
attempts fallback matching instead of returning None, and ensure logging still
records the exception (LOG.error) before falling back.
---
Nitpick comments:
In `@docs/skill-interaction.md`:
- Around line 46-49: Several fenced code blocks containing dialog snippets
(e.g., the blocks with text "Are you sure you want to delete this?", "Do you
really want to delete it?", and the other plain-text dialog blocks around the
same sections) lack language specifiers; update each triple-backtick fence to
include a language tag (use "text") so the blocks become ```text ... ```; apply
this change to the code fences containing the shown lines and the similar blocks
at the other locations referenced in the comment.
In `@ovos_workshop/skills/ovos.py`:
- Around line 1964-1982: The method _get_selection_engine currently defaults
plugin_name to "ovos-option-matcher-fuzzy-plugin" causing a load attempt on
every call; change it to mirror _get_yesno_engine by not defaulting to a plugin:
derive plugin_name only from self.settings and self.config_core (no hardcoded
default), compute cache_key the same, and only call
load_option_matcher_plugin(plugin_name) and instantiate when plugin_name is
truthy; if no plugin_name set, set the cache attribute to None so future calls
are cached and avoid repeated error logs from load_option_matcher_plugin.
In `@test/unittests/test_skill_interaction.py`:
- Around line 78-112: Add a caching test to TestGetSelectionEngine to mirror
TestGetYesnoEngine: create a new test method (e.g.
test_engine_cached_across_calls) that calls skill._get_selection_engine() twice
while patching ovos_plugin_manager.agents.load_option_matcher_plugin to return a
MagicMock and then assert the plugin load was called only once; reference
TestGetSelectionEngine, _get_selection_engine, and load_option_matcher_plugin
when locating where to add the test.
- Around line 218-224: The test test_no_engine_no_response_returns_none
incorrectly simulates a missing engine by patching _get_selection_engine to
return None; instead, add or replace a test that simulates a present engine that
yields no match by patching _get_selection_engine to return a mock engine whose
match_option method returns None, then call ask_selection(options, numeric=True)
and assert it returns None; reference the test name, _get_selection_engine,
match_option, and ask_selection so the change targets the selection flow where
the engine exists but finds no match.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dbbfb193-fd76-43c4-860b-7d60806f6338
📒 Files selected for processing (4)
docs/skill-interaction.mdovos_workshop/skills/ovos.pypyproject.tomltest/unittests/test_skill_interaction.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
ovos_workshop/skills/ovos.py (2)
1983-1987:⚠️ Potential issue | 🟠 MajorDon’t let a plugin exception take down
ask_yesno().Line 1985 calls external plugin code without a guard. A single runtime failure now bubbles out of
ask_yesno()instead of using the sameYesNoSolverfallback you already keep for load failures.Proposed fix
resp = self.get_response(dialog=prompt, data=data) engine = self._get_yesno_engine() if engine is not None: - answer = engine.yes_or_no(question=prompt, response=resp, lang=self.lang) if resp else None + try: + answer = engine.yes_or_no(question=prompt, response=resp, lang=self.lang) if resp else None + except Exception as e: + self.log.exception(f"YesNo plugin failed: {e}") + answer = YesNoSolver().match_yes_or_no(resp, lang=self.lang) if resp else resp else: answer = YesNoSolver().match_yes_or_no(resp, lang=self.lang) if resp else resp🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovos_workshop/skills/ovos.py` around lines 1983 - 1987, When calling the external plugin in ask_yesno(), wrap the call to self._get_yesno_engine().yes_or_no(...) in a try/except so a plugin exception doesn’t propagate; if an exception occurs (or the engine returns unexpectedly), log or ignore the error and fall back to using YesNoSolver().match_yes_or_no(resp, lang=self.lang) (preserving the existing resp falsy behavior where resp -> None or resp as before). Ensure you reference the existing symbols: ask_yesno, _get_yesno_engine, engine.yes_or_no, and YesNoSolver.match_yes_or_no.
1995-1997:⚠️ Potential issue | 🟠 Major
min_confis a dead parameter in the new flow.Lines 1996-1997 still expose
min_conf, but the plugin-backed implementation never reads it. Existing callers lose per-call control over selection strictness without any warning. Either thread it back into the matcher path or start deprecating the argument now.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovos_workshop/skills/ovos.py` around lines 1995 - 1997, The ask_selection method currently exposes the min_conf parameter but the plugin-backed selection path never uses it; either forward min_conf into the matcher/plugin call or formally deprecate it. Fix option A: update ask_selection to pass min_conf through to the underlying selection logic (e.g., forward min_conf into whatever method is invoked for matching/selection such as the plugin call or matcher.match invoked by the plugin-backed implementation) so per-call strictness is honored. Fix option B: if you intend to remove it, modify ask_selection to emit a DeprecationWarning when min_conf is passed (or when non-default is used), document that it will be removed, and ensure callers retain behavior by using the global/configured matcher settings. Ensure the change references ask_selection and the plugin/matcher call sites so the parameter is actually consumed or deprecated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/skill-interaction.md`:
- Around line 45-49: The Markdown contains unlabeled fenced code blocks that
show dialog examples (e.g., the block containing "Are you sure you want to
delete this?" / "Do you really want to delete it?"), causing markdownlint
warnings; update each unlabeled triple-backtick fence for those dialog examples
to use a language tag `text` (change ``` to ```text) for all similar plain-text
fenced blocks (including the other examples showing short dialog lines) so the
page passes linting.
In `@ovos_workshop/skills/ovos.py`:
- Around line 2037-2045: The code currently returns the raw transcript in resp
when _get_selection_engine() returns None; change this so the method returns
None instead in that path to match ask_selection()'s contract. Locate the block
using _get_selection_engine() and engine.match_option (around the resp handling)
and either set resp = None or early-return None when engine is None, keeping the
existing exception handling for engine.match_option intact.
---
Duplicate comments:
In `@ovos_workshop/skills/ovos.py`:
- Around line 1983-1987: When calling the external plugin in ask_yesno(), wrap
the call to self._get_yesno_engine().yes_or_no(...) in a try/except so a plugin
exception doesn’t propagate; if an exception occurs (or the engine returns
unexpectedly), log or ignore the error and fall back to using
YesNoSolver().match_yes_or_no(resp, lang=self.lang) (preserving the existing
resp falsy behavior where resp -> None or resp as before). Ensure you reference
the existing symbols: ask_yesno, _get_yesno_engine, engine.yes_or_no, and
YesNoSolver.match_yes_or_no.
- Around line 1995-1997: The ask_selection method currently exposes the min_conf
parameter but the plugin-backed selection path never uses it; either forward
min_conf into the matcher/plugin call or formally deprecate it. Fix option A:
update ask_selection to pass min_conf through to the underlying selection logic
(e.g., forward min_conf into whatever method is invoked for matching/selection
such as the plugin call or matcher.match invoked by the plugin-backed
implementation) so per-call strictness is honored. Fix option B: if you intend
to remove it, modify ask_selection to emit a DeprecationWarning when min_conf is
passed (or when non-default is used), document that it will be removed, and
ensure callers retain behavior by using the global/configured matcher settings.
Ensure the change references ask_selection and the plugin/matcher call sites so
the parameter is actually consumed or deprecated.
🪄 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: a902e2b6-665e-4509-b678-e7dc90260f69
📒 Files selected for processing (4)
docs/skill-interaction.mdovos_workshop/skills/ovos.pypyproject.tomltest/unittests/test_skill_interaction.py
✅ Files skipped from review due to trivial changes (1)
- pyproject.toml
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@ovos_workshop/skills/ovos.py`:
- Around line 1984-1986: The code passes the dialog key `prompt` to the yes/no
engine instead of the rendered question text; to fix, render the prompt with
`self.dialog_renderer.render(prompt, data)` (as `get_response` does internally)
and pass that rendered string into
`_get_yesno_engine().yes_or_no(question=rendered, response=resp,
lang=self.lang)` so the engine receives the actual question context; update
references to `prompt` when calling `yes_or_no`, keeping
`get_response(dialog=prompt, data=data)` and `resp` unchanged.
- Around line 1936-1953: The helper _get_yesno_engine currently returns a bare
YesNoSolver() on plugin load failure but ask_yesno expects an engine with
yes_or_no(question,response,lang); this mismatch causes AttributeError. Fix by
either (A) returning None from _get_yesno_engine on failure and updating
ask_yesno to detect None and call YesNoSolver().match_yes_or_no(resp, self.lang)
as the built-in fallback, or (B) implement a small adapter class (e.g.,
YesNoAdapter) that wraps YesNoSolver and exposes yes_or_no(question, response,
lang) delegating to match_yes_or_no(response, lang), and return an instance of
that adapter from _get_yesno_engine when loading the plugin fails; update
references to _get_yesno_engine/ask_yesno accordingly.
- Around line 1955-1972: The min_conf argument to ask_selection is never
forwarded to the matcher; update ask_selection to pass min_conf through to the
matcher call (e.g. call engine.match_option(..., min_conf=min_conf)) and ensure
plugin instances created in _get_selection_engine (via
load_option_matcher_plugin and fallback FuzzyOptionMatcherPlugin)
accept/configure that value (either by passing a config/min_conf into the plugin
constructor or setting an attribute like engine.min_conf after instantiation) so
the runtime matcher actually uses the provided threshold.
In `@requirements/requirements.txt`:
- Line 6: Add the missing runtime dependency for the fuzzy matcher to
requirements/requirements.txt so importing FuzzyOptionMatcherPlugin in
ovos_workshop/skills/ovos.py (the module-level import of
FuzzyOptionMatcherPlugin from ovos_option_matcher_fuzzy) no longer raises
ModuleNotFoundError; update requirements/requirements.txt to include the package
name used at runtime (ovos-option-matcher-fuzzy-plugin) with an appropriate
version specifier matching pyproject.toml.
🪄 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: 6246fbb7-dd85-4b66-82da-a12d613f22a2
📒 Files selected for processing (3)
ovos_workshop/skills/ovos.pypyproject.tomlrequirements/requirements.txt
✅ Files skipped from review due to trivial changes (1)
- pyproject.toml
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
ovos_workshop/skills/ovos.py (2)
1980-1982:⚠️ Potential issue | 🟠 MajorRender the spoken prompt and guard the yes/no plugin call.
get_response(dialog=prompt, data=data)asks the rendered dialog, butyes_or_no(question=prompt, ...)still sees the unresolved key. Also, any engine exception currently bubbles out and aborts the whole prompt flow. Use the rendered question text here and normalize plugin failures to a raw-response fallback instead of raising.Proposed fix
resp = self.get_response(dialog=prompt, data=data) - engine = self._get_yesno_engine() - answer = engine.yes_or_no(question=prompt, response=resp, lang=self.lang) if resp else None + if not resp: + return resp + + question = prompt + if self.dialog_renderer: + question = self.dialog_renderer.render(prompt, data or {}) + + engine = self._get_yesno_engine() + try: + answer = engine.yes_or_no(question=question, response=resp, + lang=self.lang) + except Exception: + LOG.exception("YesNo plugin failed") + return resp🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovos_workshop/skills/ovos.py` around lines 1980 - 1982, The code calls yes_or_no with the original prompt and lets plugin exceptions bubble up; change it to pass the rendered question text (use the returned resp from get_response as the question argument to engine.yes_or_no) and wrap the plugin call in a try/except so any exception is caught and normalized to a fallback raw-response behavior (e.g., set answer to resp or None) instead of raising; update the call site that uses _get_yesno_engine(), yes_or_no(question=...), resp, and self.lang accordingly so the rendered text is used and plugin failures return a safe fallback.
1957-1964:⚠️ Potential issue | 🟠 Major
min_confis still ignored byask_selection().Line 1991 still exposes
min_conf, but the new matcher path builds the engine with defaults and never uses that argument on the call site. Callers now silently lose control over the acceptance threshold and always get whatever default the matcher chooses.Also applies to: 2033-2035
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@ovos_workshop/skills/ovos.py` around lines 1957 - 1964, ask_selection() currently ignores the min_conf argument because the cached matcher instance (built from plugin_name via load_option_matcher_plugin and stored under cache_key) is constructed with defaults and reused; update the code so min_conf affects matching: either pass min_conf into the matcher on each call to the engine's match/select method (e.g., engine.match(options, min_conf=...)) or include min_conf in the cache key so a matcher is constructed with the desired threshold (e.g., cache_key = f"__selection_engine_{plugin_name}_{min_conf}") and ensure the matcher is instantiated with that min_conf if its constructor accepts it; modify the code paths using cache_key, load_option_matcher_plugin, and the engine invocation in ask_selection() so callers’ min_conf is respected rather than silently using the plugin defaults.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@ovos_workshop/skills/ovos.py`:
- Around line 1980-1982: The code calls yes_or_no with the original prompt and
lets plugin exceptions bubble up; change it to pass the rendered question text
(use the returned resp from get_response as the question argument to
engine.yes_or_no) and wrap the plugin call in a try/except so any exception is
caught and normalized to a fallback raw-response behavior (e.g., set answer to
resp or None) instead of raising; update the call site that uses
_get_yesno_engine(), yes_or_no(question=...), resp, and self.lang accordingly so
the rendered text is used and plugin failures return a safe fallback.
- Around line 1957-1964: ask_selection() currently ignores the min_conf argument
because the cached matcher instance (built from plugin_name via
load_option_matcher_plugin and stored under cache_key) is constructed with
defaults and reused; update the code so min_conf affects matching: either pass
min_conf into the matcher on each call to the engine's match/select method
(e.g., engine.match(options, min_conf=...)) or include min_conf in the cache key
so a matcher is constructed with the desired threshold (e.g., cache_key =
f"__selection_engine_{plugin_name}_{min_conf}") and ensure the matcher is
instantiated with that min_conf if its constructor accepts it; modify the code
paths using cache_key, load_option_matcher_plugin, and the engine invocation in
ask_selection() so callers’ min_conf is respected rather than silently using the
plugin defaults.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f7206124-7afe-447e-9804-1c884b9f9bb7
📒 Files selected for processing (3)
ovos_workshop/skills/ovos.pypyproject.tomlrequirements/requirements.txt
✅ Files skipped from review due to trivial changes (1)
- pyproject.toml
🚧 Files skipped from review as they are similar to previous changes (1)
- requirements/requirements.txt
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- patch load_yesno_plugin/load_option_matcher_plugin at ovos_workshop.skills.ovos (not ovos_plugin_manager.agents) since they are imported into the module - _get_yesno_engine falls back to HeuristicYesNoEngine, not None - _get_selection_engine falls back to FuzzyOptionMatcherPlugin, not None - replace YesNoSolver references with patch.object on _get_yesno_engine Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
build_tests already runs the same test path; unit_tests only differed by adding audio system deps not needed for these tests Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- test_base.py: fill in TODO stubs with mock-based tests on real OVOSSkill
(yes/no/unmatched/timeout for ask_yesno; empty/single/invalid/fuzzy/timeout
for ask_selection)
- test_ask_e2e.py: ovoscope end-to-end tests — inline AskYesNoSkill and
AskSelectionSkill loaded via get_minicroft; user response injected via
{skill_id}.converse.get_response after hearing speak; skipped when
ovoscope/ovos-core are not installed
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- remove pytest.importorskip — e2e tests are mandatory, not optional - add [project.optional-dependencies] test group to pyproject.toml (ovos-core, ovoscope, pytest, pytest-cov, ovos-translate-server-plugin) - build_tests.yml: pass install_extras=test so CI installs test deps - keep requirements/test.txt in sync for local pip install -r workflows Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The ask_selection min_conf parameter was documented but silently ignored. FuzzyOptionMatcherPlugin reads threshold from self.config["min_conf"], so set it on the cached engine instance before each match_option call. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
AI-Generated Change: - Model: claude-sonnet-4-6 - Intent: keep docs accurate after feat: yesno/selection agent plugins - Impact: updated README.md, docs/index.md, docs/ovos-skill.md, docs/skill-interaction.md - Changes: - README.md: rewritten with install instructions, config table, and docs links - docs/skill-interaction.md: fixed fallback class name (HeuristicYesNoEngine not YesNoSolver), corrected ask_yesno_plugin default (ovos-solver-yes-no-plugin not None), added line-number citations, corrected failure behaviour table - docs/ovos-skill.md: noted plugin configurability for ask_yesno/ask_selection - docs/index.md: added skill-interaction.md to navigation table - Verified via: manual review against ovos_workshop/skills/ovos.py:1932-2040 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
get_minicroft blocks up to 60s waiting for READY — too heavy for the standard build_tests matrix. Move test/unittests/test_ask_e2e.py to test/ovoscope/ (not scanned by build_tests) and add a dedicated ovoscope_tests workflow that installs [test] extras and runs that path. Revert install_extras from build_tests.yml. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
ask_selection speaks each option with wait=True, blocking 15s per speak waiting for recognizer_loop:audio_output_end that never arrives in tests. Emit that event immediately on every speak message so the skill proceeds without delay, then inject the user response once speaking settles. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…tests Listening on "message" caught the audio_output_end we emitted, causing infinite recursion. Listen on "speak" specifically instead. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
No need for a separate workflow. Move test_ask_e2e.py back to test/unittests/ and install [test] extras in build_tests so ovoscope and ovos-core are available. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
FakeBus is synchronous — on_speak fires inline during bus.emit(speak), before the caller sets sess.is_speaking=True. Emitting audio_output_end in the same stack means wait_while_speaking misses it and hangs 15s. Use a daemon thread with a 20ms delay so is_speaking is set first. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The tests fought TTS wait machinery (speak(wait=True) blocks on recognizer_loop:audio_output_end) without adding coverage beyond what test_skill_interaction.py already provides via mocked get_response. Unit tests are sufficient here. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
No ovoscope e2e tests remain, no need for the heavy dependency. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Patch SessionManager.wait_while_speaking to a no-op so speak(wait=True) doesn't block on TTS completion. Inject user response via skill.converse.get_response.enable event instead of timing hacks. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
get_minicroft sets SessionManager.bus globally. Without restoring it, subsequent tests get real wait_while_speaking behaviour instead of the early-return they expect (bus=None). Also scope wait_while_speaking patch to setUp/tearDown so it can't leak if tearDownClass throws. Co-Authored-By: Claude Sonnet 4.6 <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.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores