Implement updated Skill API #372
Conversation
Implements updates to the Skill API allowing the method to specify input/return values as Pydantic models. Also implements standard exception handling, reporting errors back to a caller instead of sending no return message.
WalkthroughAdds Pydantic-aware public API handling to OVOSSkill._register_public_api: optional request-model construction for handlers, serialization of Pydantic responses, enriched public_api metadata (signatures and JSON schemas), error capture in responses, and graceful fallback when Pydantic is absent. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Bus as MessageBus
participant Skill as OVOSSkill
participant Wrapper as wrap_method
participant API as api_method
Client->>Bus: Emit API call message (args/kwargs)
Bus->>Skill: Forward message
Skill->>Wrapper: Call wrapped handler (may include arg_model)
alt Request model provided
Wrapper->>Wrapper: Build model instance from message.args/kwargs
Wrapper->>API: api_method(model_instance)
else No request model
Wrapper->>API: api_method(*args, **kwargs)
end
alt Success
API-->>Wrapper: result (possibly Pydantic)
Wrapper->>Wrapper: If Pydantic -> serialize via model_dump()
Wrapper-->>Skill: { result, error: null }
else Exception
API-->>Wrapper: raises
Wrapper-->>Skill: { result: partial/null, error: "<trace or message>" }
end
Skill-->>Bus: Emit response (ensures message.context["skill_id"])
Note right of Skill: During registration, skill stored signature,<br/>request_schema, response_schema, request_class in public_api
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
ovos_workshop/skills/ovos.py (2)
1044-1045: Prefer structured error info and logging while keeping broad catchCatching Exception here is reasonable to guarantee a reply, but include error type and log the traceback for observability.
Apply:
- except Exception as e: - error = str(e) + except Exception as e: + self.log.exception("Public API handler failed") + error = f"{e.__class__.__name__}: {e}"
1051-1054: Remove redundant re-import of get_non_propertiesAlready imported at Line 44; this inner import is unnecessary.
Apply:
- from ovos_utils.skills import get_non_properties
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
ovos_workshop/skills/ovos.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
ovos_workshop/skills/ovos.py (3)
ovos_workshop/skills/api.py (2)
get(66-82)method(46-59)ovos_workshop/skills/common_play.py (1)
get_non_properties(20-42)test/unittests/test_skill_api.py (1)
add_event(15-20)
🪛 Ruff (0.12.2)
ovos_workshop/skills/ovos.py
1044-1044: Do not catch blind exception: Exception
(BLE001)
🪛 GitHub Actions: Run UnitTests
ovos_workshop/skills/ovos.py
[error] 1275-1275: Default shutdown encountered an error: sys.meta_path is None, Python is likely shutting down
🔇 Additional comments (1)
ovos_workshop/skills/ovos.py (1)
1090-1095: Public API metadata enrichment looks goodIncluding signature, request/response schemas, and stripping func/request_class later is sensible.
| 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) | ||
|
|
There was a problem hiding this comment.
💡 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_workshopLength 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/skillsLength 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/skillsLength 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.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
ovos_workshop/skills/ovos.py (3)
1113-1116: Surface API errors to callers (companion change in SkillApi)Since wrapper now always returns {'result', 'error'}, ensure SkillApi raises/handles on error.
Proposed change (ovos_workshop/skills/api.py around the method invocation):
- else: - return response.data['result'] + else: + if response.data.get('error'): + raise RuntimeError(response.data['error']) + return response.data.get('result')Quick check:
#!/bin/bash # Verify callers inspect response.data['error'] rg -n -C2 -g 'ovos_workshop/**/*.py' "response\.data\[['\"]error['\"]\]"
1026-1049: Harden wrapper: avoid KeyError, support kwargs-only BaseModels, and serialize Pydantic v1/v2 results
- Guard missing args/kwargs to prevent KeyError.
- Build arg_model robustly (merge single dict positional into kwargs; fall back to kwargs-only).
- Normalize result via model_dump() (v2) or dict() (v1).
- Keep broad catch but silence Ruff BLE001 locally.
Apply:
def wrap_method(fn, arg_model=None): """Boilerplate for returning the response to the sender.""" def wrapper(message): - 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) + result = None + error = None + try: + args = (message.data.get('args') or []) + kwargs = (message.data.get('kwargs') or {}) + if arg_model: + # Merge single-dict positional arg into kwargs + if len(args) == 1 and isinstance(args[0], dict): + kwargs = {**args[0], **kwargs} + args = [] + try: + req = arg_model(*args, **kwargs) + except TypeError: + # Many BaseModels are kwargs-only + req = arg_model(**kwargs) + result = fn(req) + else: + result = fn(*args, **kwargs) + # Normalize Pydantic v2/v1 models + if hasattr(result, "model_dump"): + result = result.model_dump() + elif hasattr(result, "dict"): + result = result.dict() + except Exception as e: # noqa: BLE001 + error = str(e) message.context["skill_id"] = self.skill_id self.bus.emit(message.response(data={'result': result, 'error': error}))
1098-1103: Bug: public_api['signature'] stores the function object, not the method signatureUse str(sig), not str(signature). Current value serializes the imported function, not the inspected signature.
self.public_api[name] = { 'help': doc, 'type': f'{self.skill_id}.{name}', 'func': method, - 'signature': str(signature), + 'signature': str(sig), 'request_schema': schema, 'response_schema': return_schema, 'request_class': request_class }
🧹 Nitpick comments (2)
ovos_workshop/skills/ovos.py (2)
1051-1051: Remove duplicate import of get_non_propertiesAlready imported at module scope (Line 44). The local import is unnecessary.
- from ovos_utils.skills import get_non_properties
1062-1093: Schema introspection: unwrap Optional/Union[BaseModel] and typing “| None” formsHandle annotations like Optional[Req], Union[Req, None], and Req | None by unwrapping to the underlying BaseModel before schema extraction.
import inspect sig = inspect.signature(method) schema = None return_schema = None request_class = None try: from pydantic import BaseModel + from typing import get_origin, get_args parameters = sig.parameters - - for arg_name, param in parameters.items(): + def _resolve_model(ann): + origin = get_origin(ann) + if origin is None: + return ann if isinstance(ann, type) and issubclass(ann, BaseModel) else None + for a in get_args(ann) or (): + if isinstance(a, type) and issubclass(a, BaseModel): + return a + return None + + for arg_name, param in parameters.items(): if arg_name == 'self': continue - ann = param.annotation - if isinstance(ann, type) and issubclass(ann, BaseModel): + ann = _resolve_model(param.annotation) + if ann: # 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): + ra = _resolve_model(sig.return_annotation) + if ra: # Get the JSON schema for the return type try: return_schema = ra.model_json_schema() except AttributeError: return_schema = ra.schema()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
ovos_workshop/skills/ovos.py(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
ovos_workshop/skills/ovos.py (3)
ovos_workshop/skills/api.py (2)
get(66-82)method(46-59)ovos_workshop/skills/common_play.py (1)
get_non_properties(20-42)test/unittests/test_skill_api.py (1)
add_event(15-20)
🪛 Ruff (0.12.2)
ovos_workshop/skills/ovos.py
1044-1044: Do not catch blind exception: Exception
(BLE001)
🔇 Additional comments (1)
ovos_workshop/skills/ovos.py (1)
1113-1116: LGTM: wire request_class through wrapperPassing req_class into wrap_method and removing it from public_api is correct.
Summary by CodeRabbit
New Features
Robustness