Skip to content

Implement updated Skill API #372

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: dev
Choose a base branch
from
Open

Implement updated Skill API #372

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9db6596
Implement updated Skill API
NeonDaniel Aug 19, 2025
199c05f
Implement CodeRabbit suggested changes
NeonDaniel Sep 3, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
67 changes: 61 additions & 6 deletions ovos_workshop/skills/ovos.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1023,16 +1023,32 @@ def _register_public_api(self):
messagebus handler for fetching the api info if any handlers exist.
"""

def wrap_method(fn):
def wrap_method(fn, arg_model=None):
"""Boilerplate for returning the response to the sender."""

def wrapper(message):
result = fn(*message.data['args'], **message.data['kwargs'])
result = None
error = None
try:
if arg_model:
result = fn(arg_model(*message.data['args'],
**message.data['kwargs']))
else:
result = fn(*message.data.get('args', []),
**message.data.get('kwargs', {}))
try:
result = result.model_dump()
except AttributeError:
# Response is not a Pydantic model
pass
except Exception as e:
error = str(e)
message.context["skill_id"] = self.skill_id
self.bus.emit(message.response(data={'result': result}))

self.bus.emit(message.response(data={'result': result,
'error': error}))
return wrapper

from ovos_utils.skills import get_non_properties
methods = [attr_name for attr_name in get_non_properties(self)
if hasattr(getattr(self, attr_name), '__name__')]

Expand All @@ -1042,10 +1058,48 @@ def wrapper(message):
if hasattr(method, 'api_method'):
doc = method.__doc__ or ''
name = method.__name__

# Extract method signature and return type
import inspect
sig = inspect.signature(method)
schema = None
return_schema = None
request_class = None
try:
from pydantic import BaseModel
parameters = sig.parameters

for arg_name, param in parameters.items():
if arg_name == 'self':
continue
ann = param.annotation
if isinstance(ann, type) and issubclass(ann, BaseModel):
# Get the JSON schema for the BaseModel
try:
schema = ann.model_json_schema()
except AttributeError:
schema = ann.schema()
request_class = ann
break
ra = sig.return_annotation
if isinstance(ra, type) and issubclass(ra, BaseModel):
# Get the JSON schema for the return type
try:
return_schema = ra.model_json_schema()
except AttributeError:
return_schema = ra.schema()
except ImportError:
# If pydantic is not installed, there is no schema to extract
pass

self.public_api[name] = {
'help': doc,
'type': f'{self.skill_id}.{name}',
'func': method
'func': method,
'signature': str(signature),
'request_schema': schema,
'response_schema': return_schema,
'request_class': request_class
}
for key in self.public_api:
if ('type' in self.public_api[key] and
Expand All @@ -1056,8 +1110,9 @@ def wrapper(message):
# remove the function member since it shouldn't be
# reused and can't be sent over the messagebus
func = self.public_api[key].pop('func')
req_class = self.public_api[key].pop('request_class', None)
self.add_event(self.public_api[key]['type'],
wrap_method(func), speak_errors=False)
wrap_method(func, req_class), speak_errors=False)

Comment on lines +1113 to 1116
Copy link
Contributor

@coderabbitai coderabbitai bot Sep 2, 2025

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Wiring handler with request_class is correct; ensure clients surface errors

wrap_method now always returns a response with result/error. Verify client-side SkillApi callers read and act on error, not just result.

Run to confirm current client behavior:

If no handling of 'error' is found in callers (e.g., ovos_workshop/skills/api.py), consider updating them to raise or return a structured exception when 'error' is present.

🏁 Script executed:

#!/bin/bash
# Check if API client surfaces 'error' from responses
rg -nP -C2 'wait_for_response\(|response\.data\[(\"|\')result|response\.data\[(\"|\')error' ovos_workshop

Length of output: 288

🏁 Script executed:

#!/bin/bash
# Search for wait_for_response in SkillApi implementations
rg -n --color=never 'wait_for_response' -g '*.py' ovos_workshop/skills

# Search for any handling of response.data["error"]
rg -n --color=never -g '*.py' 'response\.data\[[\"\']error[\"\']\]' ovos_workshop/skills

# Search for any handling of response.data["result"]
rg -n --color=never -g '*.py' 'response\.data\[[\"\']result[\"\']\]' ovos_workshop/skills

Length of output: 508

🏁 Script executed:

#!/bin/bash
# Search for wait_for_response usage
rg -n --color=never 'wait_for_response' -g '*.py' ovos_workshop/skills

# Search for any direct inspection/handling of response.data["error"]
rg -n --color=never -g '*.py' "response\\.data\\[['\"]error['\"]\\]" ovos_workshop/skills

# Search for any direct inspection/handling of response.data["result"]
rg -n --color=never -g '*.py' "response\\.data\\[['\"]result['\"]\\]" ovos_workshop/skills

Length of output: 664

Handle API errors in SkillApi

SkillApi (ovos_workshop/skills/api.py:59) unconditionally returns response.data['result'] without checking response.data['error']. Introduce error handling such that if error is present the client raises or returns a structured exception—for example:

# ovos_workshop/skills/api.py around line 59
if 'error' in response.data:
    raise SkillApiError(response.data['error'])
return response.data['result']
🤖 Prompt for AI Agents 
In ovos_workshop/skills/api.py around line 59, the SkillApi currently returns
response.data['result'] without checking for errors; change the logic to inspect
response.data for an 'error' key and if present raise a SkillApiError (or return
a structured exception object) built from response.data['error'] (ensure
SkillApiError is defined/imported and contains the relevant fields), otherwise
continue to return response.data['result']; keep exception raising consistent
with existing client error handling semantics.

if self.public_api:
self.add_event(f'{self.skill_id}.public_api',
Expand Down
Loading