Conversation
|
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:
📝 WalkthroughWalkthroughAdds a ToolBox framework exposing agent tools over the message bus (Pydantic input/output models, AgentTool dataclass, type aliases), an abstract ToolBox with discovery/call handlers and validation, a toolbox discovery helper, new PluginTypes entries for agent toolboxes, documentation, tests, and Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant Bus
participant ToolBox
participant Discover as discover_tools()
Caller->>ToolBox: instantiate(toolbox_id, bus?)
ToolBox->>Discover: discover_tools()
Discover-->>ToolBox: List[AgentTool]
ToolBox->>ToolBox: cache tools
alt bus provided
ToolBox->>Bus: bind(discovery_channel, call_channel)
end
Note over Caller,Bus: Discovery flow
Caller->>Bus: publish(discovery_channel)
Bus->>ToolBox: handle_discover(msg)
ToolBox->>ToolBox: tool_json_list
ToolBox-->>Bus: emit(toolbox_id, tool_schemas)
Bus-->>Caller: tool list
Note over Caller,Bus: Call flow
Caller->>Bus: publish(call_channel, tool_name, kwargs)
Bus->>ToolBox: handle_call(msg)
ToolBox->>ToolBox: call_tool(name, kwargs)
alt tool exists & success
ToolBox-->>Bus: emit(result)
else not found / error
ToolBox-->>Bus: emit(error)
end
Bus-->>Caller: response
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)
📝 Coding Plan
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
ovos_plugin_manager/templates/agent_tools.py (1)
158-172: Consider logging lazy tool discovery for observability.The lazy loading pattern is sound, but operators may benefit from knowing when tool discovery is deferred from initialization to first usage.
Apply this diff to add optional debug logging:
+from ovos_utils.log import LOG + def get_tool(self, name: str) -> Optional[AgentTool]: """...""" if name not in self.tools: + LOG.debug(f"Tool '{name}' not in cache, refreshing tools for ToolBox '{self.toolbox_id}'") self.refresh_tools() return self.tools.get(name)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ovos_plugin_manager/templates/agent_tools.py(1 hunks)requirements/requirements.txt(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
ovos_plugin_manager/templates/agent_tools.py (1)
ovos_plugin_manager/templates/phal.py (1)
emit(138-141)
🪛 Ruff (0.14.0)
ovos_plugin_manager/templates/agent_tools.py
61-62: try-except-pass detected, consider logging the exception
(S110)
61-61: Do not catch blind exception: Exception
(BLE001)
61-61: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
124-124: Do not catch blind exception: Exception
(BLE001)
126-126: Use explicit conversion flag
Replace with conversion flag
(RUF010)
154-154: Avoid specifying long messages outside the exception class
(TRY003)
156-156: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: unit_tests (3.10)
- GitHub Check: unit_tests (3.11)
- GitHub Check: build_tests (3.10)
🔇 Additional comments (9)
ovos_plugin_manager/templates/agent_tools.py (9)
10-19: LGTM!The empty base model pattern is appropriate for framework extension points, allowing plugin developers to define their own tool argument and output schemas.
22-34: LGTM!The dataclass structure cleanly separates declarative metadata (schemas, description) from executable logic (tool_call). The flexible callable signature appropriately accommodates diverse tool implementations.
83-89: LGTM!The tool cache refresh logic is clean and idempotent, appropriately delegating to the abstract
discover_tools()method.
91-105: LGTM!The discovery handler correctly returns tool schemas as JSON, making tools discoverable over the message bus. The property access for
tool_json_listis appropriate.
107-127: LGTM!The message bus handler correctly delegates to
call_tool()and emits both success and error responses. The broad exception catch is appropriate for a message bus integration point, ensuring the bus remains stable even if tool execution fails.
129-156: LGTM!The direct execution interface provides clear error handling with proper exception chaining. The descriptive error messages aid debugging by including tool name and toolbox context.
174-194: LGTM!The JSON schema generation correctly leverages Pydantic's
model_json_schema()to produce LLM-compatible tool definitions. The output format is well-structured for both message bus transmission and LLM API consumption.
197-208: LGTM!The abstract method contract is clear and well-documented. The idempotency requirement appropriately supports the refresh/lazy-loading design.
68-81: ---No issues identified with message bus event naming.
The
ovos.persona.tools.*patterns align with existing OVOS conventions found throughout the codebase. Similar hierarchical dot-notation patterns are used consistently in other components (e.g.,mycroft.gui.screen.close,enclosure.system.reset), confirming the naming convention is appropriate for the plugin manager integration.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
ovos_plugin_manager/persona.py (1)
16-22: Consider clarifying the docstring terminology.The implementation is correct and follows the established pattern. However, the docstring mentions "toolbox_id to entrypoints (ToolBox)" which could be clearer. The function returns a mapping of plugin names (which serve as toolbox identifiers) to ToolBox class types, not entry point objects.
Consider this docstring refinement:
def find_toolbox_plugins() -> Dict[str, Type[ToolBox]]: """ Find all installed Toolbox plugins - @return: dict toolbox_id to entrypoints (ToolBox) + @return: dict mapping plugin names to ToolBox class types """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ovos_plugin_manager/persona.py(1 hunks)ovos_plugin_manager/utils/__init__.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
ovos_plugin_manager/persona.py (2)
ovos_plugin_manager/templates/agent_tools.py (1)
ToolBox(37-208)ovos_plugin_manager/utils/__init__.py (2)
PluginTypes(51-95)find_plugins(143-172)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: unit_tests (3.11)
- GitHub Check: unit_tests (3.10)
🔇 Additional comments (4)
ovos_plugin_manager/utils/__init__.py (1)
95-95: LGTM! Enum addition follows existing patterns.The new
PERSONA_TOOLenum member is correctly placed, follows the naming convention, and uses the appropriate "opm.*" identifier pattern. The absence of a correspondingPluginConfigTypesentry is consistent with thePERSONAtype, which also has no config entry.ovos_plugin_manager/persona.py (3)
1-3: LGTM! Imports are correct and necessary.The typing imports support the new type annotations, and the
ToolBoximport aligns with the new toolbox plugin framework introduced in this PR.
7-9: LGTM! Type annotation and docstring improvements enhance code clarity.The return type annotation accurately reflects that persona plugins are dictionary-based configurations, and the updated docstring ("definitions" vs "plugins") is more precise terminology.
3-3: Pydantic dependency is properly declared.The verification confirms that
pydantic~=2.0is listed in the project's requirements file, which satisfies the requirement forToolBoxto use Pydantic 2.0+ features likemodel_json_schema().
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
ovos_plugin_manager/templates/agent_tools.py (1)
64-67: Add logging to avoid silent initialization failures.This silent exception swallowing was previously flagged and remains unaddressed. While lazy loading provides a fallback (line 236), silently hiding initialization errors (missing dependencies, import failures, configuration issues) makes debugging significantly harder for operators.
Apply this diff to add logging:
+from ovos_utils.log import LOG + # (in __init__ method) try: self.discover_tools() # try to find tools immediately -except Exception as e: - pass # will be lazy loaded or throw error on first usage +except Exception: + LOG.warning(f"Failed to discover tools for ToolBox '{toolbox_id}' during init", exc_info=True) + # will be lazy loaded or throw error on first usageNote: Removed the unused variable
eand usedexc_info=Trueto capture the full traceback.Based on past review comments.
🧹 Nitpick comments (1)
ovos_plugin_manager/templates/agent_tools.py (1)
96-132: LGTM!The message bus handlers are correctly implemented. The broad exception catching on line 129 is appropriate for a message bus handler—it ensures all errors are captured and returned as error responses rather than crashing the handler.
Minor style note: Line 131 could use explicit
str()conversion for clarity:- error: str = f"{type(e).__name__}: {str(e)}" + error: str = f"{type(e).__name__}: {e!s}"However, this is purely stylistic and the current code is functionally correct.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ovos_plugin_manager/templates/agent_tools.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
ovos_plugin_manager/templates/agent_tools.py (1)
ovos_plugin_manager/plugin_entry.py (2)
name(51-52)description(82-85)
🪛 Ruff (0.14.0)
ovos_plugin_manager/templates/agent_tools.py
66-67: try-except-pass detected, consider logging the exception
(S110)
66-66: Do not catch blind exception: Exception
(BLE001)
66-66: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
129-129: Do not catch blind exception: Exception
(BLE001)
131-131: Use explicit conversion flag
Replace with conversion flag
(RUF010)
154-154: Avoid specifying long messages outside the exception class
(TRY003)
177-177: Avoid specifying long messages outside the exception class
(TRY003)
206-206: Avoid specifying long messages outside the exception class
(TRY003)
213-213: Avoid specifying long messages outside the exception class
(TRY003)
220-220: Avoid specifying long messages outside the exception class
(TRY003)
222-222: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: unit_tests (3.11)
- GitHub Check: unit_tests (3.10)
🔇 Additional comments (5)
ovos_plugin_manager/templates/agent_tools.py (5)
1-20: LGTM!The imports and base Pydantic models are well-structured. Using empty base classes for
ToolArgumentsandToolOutputis a valid pattern that allows concrete toolbox plugins to define their own schemas while maintaining type consistency.
23-40: LGTM!The
ToolCallFunctype alias andAgentTooldataclass are well-designed. The use of field metadata for documentation is excellent, and the type signature is used consistently throughout the codebase (e.g., line 210).
73-94: LGTM!The message bus binding and tool refresh logic is correctly implemented. The lack of exception handling in
refresh_tools()is appropriate since it's called on-demand and errors should propagate to the caller.
134-177: LGTM!Both validation methods correctly use Pydantic v2 APIs (
model_validate()on line 175) and provide informative error messages that include the tool name and problematic data for debugging.
180-275: LGTM!The tool execution orchestration is well-designed with clear separation of concerns:
- Input validation failures raise
ValueError(line 206)- Execution failures raise
RuntimeError(line 213)- Output validation failures raise
RuntimeError(line 220)This error categorization helps callers distinguish between validation issues (caller's fault) and runtime issues (tool implementation's fault). The lazy loading in
get_tool()(lines 236-237) and the use of Pydantic v2'smodel_json_schema()(lines 257-258) are both correct.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
ovos_plugin_manager/templates/agent_tools.py (1)
66-69: Address the exception logging as noted in previous review.This silent exception swallowing was already flagged. Please implement the suggested logging to aid debugging of initialization failures.
🧹 Nitpick comments (1)
ovos_plugin_manager/templates/agent_tools.py (1)
181-240: LGTM! Comprehensive tool execution with thorough validation.The execution orchestration is well-structured with clear separation of concerns: input validation, execution, and output validation. The error handling provides good context at each stage.
Optional refinement: Lines 223-226 and 237-240 could use
TypeErrorinstead ofRuntimeErrorfor type mismatches, as it more precisely indicates the nature of the error.Optional diff for semantic precision:
if isinstance(raw_or_validated_result, ToolOutput): # Case A: Tool returned an already validated Pydantic model. # We perform a quick type check to ensure it matches the declared schema. if not isinstance(raw_or_validated_result, tool.output_schema): - raise RuntimeError( + raise TypeError( f"Tool '{name}' returned model of type {type(raw_or_validated_result).__name__}, " f"but expected {tool.output_schema.__name__}." ) return raw_or_validated_result elif isinstance(raw_or_validated_result, dict): # Case B: Tool returned a raw dictionary (needs validation). try: return self.validate_output(tool, raw_or_validated_result) except ValueError as e: # Catch Pydantic output ValidationErrors raise RuntimeError(f"Tool output validation failed for '{name}' in ToolBox '{self.toolbox_id}'") from e else: # Case C: Tool returned an unexpected type. - raise RuntimeError( + raise TypeError( f"Tool '{name}' returned an unexpected type: {type(raw_or_validated_result).__name__}. " "Must return Dict[str, Any] or ToolOutput." )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ovos_plugin_manager/templates/agent_tools.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.14.0)
ovos_plugin_manager/templates/agent_tools.py
68-69: try-except-pass detected, consider logging the exception
(S110)
68-68: Do not catch blind exception: Exception
(BLE001)
68-68: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
131-131: Do not catch blind exception: Exception
(BLE001)
133-133: Use explicit conversion flag
Replace with conversion flag
(RUF010)
156-156: Avoid specifying long messages outside the exception class
(TRY003)
179-179: Avoid specifying long messages outside the exception class
(TRY003)
202-202: Avoid specifying long messages outside the exception class
(TRY003)
209-209: Avoid specifying long messages outside the exception class
(TRY003)
216-216: Avoid specifying long messages outside the exception class
(TRY003)
223-226: Prefer TypeError exception for invalid type
(TRY004)
223-226: Avoid specifying long messages outside the exception class
(TRY003)
234-234: Avoid specifying long messages outside the exception class
(TRY003)
237-240: Prefer TypeError exception for invalid type
(TRY004)
237-240: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: unit_tests (3.10)
- GitHub Check: unit_tests (3.11)
- GitHub Check: build_tests (3.10)
🔇 Additional comments (5)
ovos_plugin_manager/templates/agent_tools.py (5)
1-19: LGTM! Clean foundation for the ToolBox framework.The imports are appropriate, and the empty Pydantic base classes provide clear extension points for concrete tool implementations.
22-41: LGTM! Well-structured tool definition.The type aliases improve clarity, and the
AgentTooldataclass provides a clean, self-documenting structure for tool metadata and execution logic.
90-134: LGTM! Solid messagebus integration and error handling.The refresh, discovery, and execution handlers are well-implemented. The broad exception catch at line 131 is appropriate for messagebus handlers to ensure graceful error responses without crashing.
136-179: LGTM! Robust validation with clear error context.Both validation methods provide clear error messages that will aid debugging. The error message detail flagged by Ruff is actually valuable for troubleshooting validation failures.
242-293: LGTM! Well-designed lazy loading and schema generation.The lazy loading pattern in
get_tool()ensures tools are discovered on-demand, and thetool_json_listproperty correctly leverages Pydantic'smodel_json_schema()for LLM-compatible tool definitions. The abstractdiscover_tools()method properly enforces the plugin contract.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
ovos_plugin_manager/templates/agent_tools.py (1)
66-69: Log initialization failures to aid debugging.This issue was already identified in a previous review. Silently swallowing exceptions during
discover_tools()can hide critical setup errors.
🧹 Nitpick comments (2)
ovos_plugin_manager/templates/agent_tools.py (2)
75-88: Consider adding an unbind/cleanup method.The
bindmethod registers message bus handlers, but there's no corresponding method to unregister them. This could lead to resource leaks or duplicate handlers if a ToolBox instance is re-bound or needs to be cleanly shut down.Consider adding an
unbindmethod:def unbind(self) -> None: """ Unregisters messagebus handlers and cleans up the connection. """ if self.bus: self.bus.remove("ovos.persona.tools.discover", self.handle_discover) self.bus.remove(f"ovos.persona.tools.{self.toolbox_id}.call", self.handle_call) self.bus = None
209-212: Prefer TypeError for type validation failures.The code raises
ValueErrorfor type mismatches, but Python convention (and Ruff TRY004) suggests usingTypeErrorwhen the issue is specifically an incorrect type rather than an invalid value.Apply this diff to use the more semantically appropriate exception type:
if not isinstance(tool_kwargs, tool.argument_schema): - raise ValueError( + raise TypeError( f"Tool '{name}' called with model of type {type(tool_kwargs).__name__}, " f"but expected {tool.argument_schema.__name__}." )else: # Case C: Input is an unexpected type. - raise RuntimeError( + raise TypeError( f"Tool '{name}' called with unexpected type arguments: {type(tool_kwargs).__name__}. " "Must be Dict[str, Any] or ToolArguments." )if not isinstance(raw_or_validated_result, tool.output_schema): - raise RuntimeError( + raise TypeError( f"Tool '{name}' returned model of type {type(raw_or_validated_result).__name__}, " f"but expected {tool.output_schema.__name__}." )else: # Case C: Tool returned an unexpected type. - raise RuntimeError( + raise TypeError( f"Tool '{name}' returned an unexpected type: {type(raw_or_validated_result).__name__}. " "Must return Dict[str, Any] or ToolOutput." )Also applies to: 223-226, 240-243, 254-257
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ovos_plugin_manager/templates/agent_tools.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.14.0)
ovos_plugin_manager/templates/agent_tools.py
68-69: try-except-pass detected, consider logging the exception
(S110)
68-68: Do not catch blind exception: Exception
(BLE001)
68-68: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
131-131: Do not catch blind exception: Exception
(BLE001)
133-133: Use explicit conversion flag
Replace with conversion flag
(RUF010)
156-156: Avoid specifying long messages outside the exception class
(TRY003)
179-179: Avoid specifying long messages outside the exception class
(TRY003)
202-202: Avoid specifying long messages outside the exception class
(TRY003)
209-212: Prefer TypeError exception for invalid type
(TRY004)
209-212: Avoid specifying long messages outside the exception class
(TRY003)
220-220: Avoid specifying long messages outside the exception class
(TRY003)
223-226: Prefer TypeError exception for invalid type
(TRY004)
223-226: Avoid specifying long messages outside the exception class
(TRY003)
233-233: Avoid specifying long messages outside the exception class
(TRY003)
240-243: Prefer TypeError exception for invalid type
(TRY004)
240-243: Avoid specifying long messages outside the exception class
(TRY003)
251-251: Avoid specifying long messages outside the exception class
(TRY003)
254-257: Prefer TypeError exception for invalid type
(TRY004)
254-257: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: unit_tests (3.10)
- GitHub Check: unit_tests (3.11)
🔇 Additional comments (5)
ovos_plugin_manager/templates/agent_tools.py (5)
1-24: LGTM! Clean setup with appropriate abstractions.The imports, base Pydantic models, and type aliases are well-structured. Using empty base classes for
ToolArgumentsandToolOutputis the correct pattern for creating a type hierarchy that concrete implementations can extend.
27-41: LGTM! Well-designed tool metadata structure.The
AgentTooldataclass provides clear separation between metadata (schemas, descriptions) and execution logic (tool_call). The field metadata is a nice touch for documentation.
90-134: LGTM! Message bus handlers are resilient and well-structured.The handlers appropriately catch exceptions and emit responses/errors back on the bus. The broad exception handling at line 131 is justified for a message bus handler that needs to remain resilient and always respond.
136-179: LGTM! Validation methods provide clear error context.Both validation methods correctly leverage Pydantic's validation and re-raise with additional context. The detailed error messages (flagged by Ruff TRY003) are actually beneficial for debugging tool integration issues.
275-310: LGTM! Clean JSON schema export and abstract method definition.The
tool_json_listproperty correctly leverages Pydantic'smodel_json_schema()to generate LLM-compatible tool definitions. The abstractdiscover_toolsmethod provides a clear contract for plugin implementations.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
ovos_plugin_manager/templates/agent_tools.py (2)
260-274: Consider preventing repeated refreshes for non-existent tools.The past review comment about inefficient repeated refreshes for non-existent tools is still valid. Consider adding a flag to track whether
refresh_tools()has already been called.
66-69: Log initialization failures to aid debugging.Silently swallowing exceptions during
discover_tools()can hide critical setup errors. The past review comment requesting logging is still valid and should be addressed.
🧹 Nitpick comments (1)
ovos_plugin_manager/templates/agent_tools.py (1)
210-213: Consider using TypeError for type validation failures.For better semantic clarity, consider using
TypeErrorinstead ofValueErrororRuntimeErrorwhen checking type compatibility (lines 210, 224, 241, 255). This would align with Python conventions whereTypeErrorindicates an operation received an inappropriate type.Example for line 210:
- raise ValueError( + raise TypeError( f"Tool '{name}' called with model of type {type(tool_kwargs).__name__}, " f"but expected {tool.argument_schema.__name__}." )Apply similar changes to lines 224, 241, and 255.
Also applies to: 224-227, 241-244, 255-258
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ovos_plugin_manager/templates/agent_tools.py(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
ovos_plugin_manager/templates/agent_tools.py (1)
ovos_plugin_manager/templates/phal.py (1)
emit(138-141)
🪛 Ruff (0.14.0)
ovos_plugin_manager/templates/agent_tools.py
68-69: try-except-pass detected, consider logging the exception
(S110)
68-68: Do not catch blind exception: Exception
(BLE001)
68-68: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
131-131: Do not catch blind exception: Exception
(BLE001)
133-133: Use explicit conversion flag
Replace with conversion flag
(RUF010)
156-156: Avoid specifying long messages outside the exception class
(TRY003)
179-179: Avoid specifying long messages outside the exception class
(TRY003)
202-202: Avoid specifying long messages outside the exception class
(TRY003)
210-213: Prefer TypeError exception for invalid type
(TRY004)
210-213: Avoid specifying long messages outside the exception class
(TRY003)
221-221: Avoid specifying long messages outside the exception class
(TRY003)
224-227: Prefer TypeError exception for invalid type
(TRY004)
224-227: Avoid specifying long messages outside the exception class
(TRY003)
234-234: Avoid specifying long messages outside the exception class
(TRY003)
241-244: Prefer TypeError exception for invalid type
(TRY004)
241-244: Avoid specifying long messages outside the exception class
(TRY003)
252-252: Avoid specifying long messages outside the exception class
(TRY003)
255-258: Prefer TypeError exception for invalid type
(TRY004)
255-258: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: unit_tests (3.11)
- GitHub Check: unit_tests (3.10)
🔇 Additional comments (7)
ovos_plugin_manager/templates/agent_tools.py (7)
1-20: LGTM! Clean base model definitions.The imports and base Pydantic models are well-structured and appropriate for the ToolBox framework.
22-41: LGTM! Well-designed tool metadata structure.The type aliases and
AgentTooldataclass provide a clean, type-safe interface for defining agent tools with proper schema validation.
75-96: LGTM! Clean message bus integration.The
bind()andrefresh_tools()methods properly set up message bus handlers and tool caching.
98-134: LGTM! Robust message bus handlers.Both
handle_discover()andhandle_call()properly handle message bus interactions with appropriate error handling. The broad exception catch inhandle_call()is justified to ensure failures are communicated back over the bus.
136-179: LGTM! Proper validation with clear error messages.Both validation methods correctly leverage Pydantic's validation capabilities and provide helpful error context.
276-297: LGTM! Clean JSON schema generation.The
tool_json_listproperty correctly uses Pydantic v2'smodel_json_schema()method to generate LLM-compatible tool definitions.
299-311: LGTM! Proper abstract method definition.The
discover_tools()abstract method is correctly defined with clear documentation about implementation requirements, including the important note about idempotency.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
ovos_plugin_manager/templates/agent_tools.py (1)
66-69: Log initialization failures to aid debugging.This issue was flagged in a previous review but remains unaddressed. Silently swallowing all exceptions during
discover_tools()can hide critical setup errors (missing dependencies, configuration issues, import failures). While lazy loading provides a fallback, operators need visibility when initialization fails.Apply this diff to add logging:
+from ovos_utils.log import LOG + # (in __init__ method) try: self.discover_tools() # try to find tools immediately except Exception as e: - pass # will be lazy loaded or throw error on first usage + LOG.warning(f"Failed to discover tools for ToolBox '{toolbox_id}' during init: {e}") + # will be lazy loaded or throw error on first usageBased on learnings
🧹 Nitpick comments (1)
ovos_plugin_manager/templates/agent_tools.py (1)
205-226: Consider usingTypeErrorfor type validation failures.When validating input types,
TypeErroris more semantically appropriate thanValueErrororRuntimeError. This applies to lines 209-212 (type mismatch for ToolArguments), 223-226 (unexpected input type), and similar patterns in the output validation section (lines 240-243, 254-257).Apply this diff to improve semantic precision:
if isinstance(tool_kwargs, ToolArguments): # Case A: Input is an already validated Pydantic model. # We perform a quick type check to ensure it matches the declared schema. if not isinstance(tool_kwargs, tool.argument_schema): - raise ValueError( + raise TypeError( f"Tool '{name}' called with model of type {type(tool_kwargs).__name__}, " f"but expected {tool.argument_schema.__name__}." ) validated_args: ToolArguments = tool_kwargs elif isinstance(tool_kwargs, dict): # Case B: Input is a raw dictionary (needs validation). try: validated_args: ToolArguments = self.validate_input(tool, tool_kwargs) except ValueError as e: # Re-raise with more context raise ValueError(f"Tool input validation failed for '{name}' in ToolBox '{self.toolbox_id}'") from e else: # Case C: Input is an unexpected type. - raise RuntimeError( + raise TypeError( f"Tool '{name}' called with unexpected type arguments: {type(tool_kwargs).__name__}. " "Must be Dict[str, Any] or ToolArguments." )Apply similar changes to output validation:
# 3. Output Validation/Casting if isinstance(raw_or_validated_result, ToolOutput): # Case A: Tool returned an already validated Pydantic model. # We perform a quick type check to ensure it matches the declared schema. if not isinstance(raw_or_validated_result, tool.output_schema): - raise RuntimeError( + raise TypeError( f"Tool '{name}' returned model of type {type(raw_or_validated_result).__name__}, " f"but expected {tool.output_schema.__name__}." ) return raw_or_validated_result elif isinstance(raw_or_validated_result, dict): # Case B: Tool returned a raw dictionary (needs validation). try: return self.validate_output(tool, raw_or_validated_result) except ValueError as e: # Catch Pydantic output ValidationErrors raise RuntimeError(f"Tool output validation failed for '{name}' in ToolBox '{self.toolbox_id}'") from e else: # Case C: Tool returned an unexpected type. - raise RuntimeError( + raise TypeError( f"Tool '{name}' returned an unexpected type: {type(raw_or_validated_result).__name__}. " "Must return Dict[str, Any] or ToolOutput." )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
ovos_plugin_manager/templates/agent_tools.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-17T15:18:51.714Z
Learnt from: JarbasAl
PR: OpenVoiceOS/ovos-plugin-manager#340
File: ovos_plugin_manager/templates/agent_tools.py:260-274
Timestamp: 2025-10-17T15:18:51.714Z
Learning: In ovos_plugin_manager/templates/agent_tools.py, the ToolBox.get_tool() method intentionally calls refresh_tools() on every cache miss to support dynamic tool discovery in MCP (Model Context Protocol) and UTCP server plugins, where tools can be added or modified at runtime. Do not suggest caching the discovery state to avoid repeated refreshes.
Applied to files:
ovos_plugin_manager/templates/agent_tools.py
🪛 Ruff (0.14.0)
ovos_plugin_manager/templates/agent_tools.py
68-69: try-except-pass detected, consider logging the exception
(S110)
68-68: Do not catch blind exception: Exception
(BLE001)
68-68: Local variable e is assigned to but never used
Remove assignment to unused variable e
(F841)
131-131: Do not catch blind exception: Exception
(BLE001)
133-133: Use explicit conversion flag
Replace with conversion flag
(RUF010)
156-156: Avoid specifying long messages outside the exception class
(TRY003)
179-179: Avoid specifying long messages outside the exception class
(TRY003)
202-202: Avoid specifying long messages outside the exception class
(TRY003)
209-212: Prefer TypeError exception for invalid type
(TRY004)
209-212: Avoid specifying long messages outside the exception class
(TRY003)
220-220: Avoid specifying long messages outside the exception class
(TRY003)
223-226: Prefer TypeError exception for invalid type
(TRY004)
223-226: Avoid specifying long messages outside the exception class
(TRY003)
233-233: Avoid specifying long messages outside the exception class
(TRY003)
240-243: Prefer TypeError exception for invalid type
(TRY004)
240-243: Avoid specifying long messages outside the exception class
(TRY003)
251-251: Avoid specifying long messages outside the exception class
(TRY003)
254-257: Prefer TypeError exception for invalid type
(TRY004)
254-257: Avoid specifying long messages outside the exception class
(TRY003)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build_tests (3.11)
🔇 Additional comments (6)
ovos_plugin_manager/templates/agent_tools.py (6)
1-19: LGTM! Clean foundation for the ToolBox framework.The imports and base Pydantic models provide a solid foundation for type-safe tool definitions with schema validation.
22-41: LGTM! Well-structured tool definition.The type aliases enhance readability, and the
AgentTooldataclass provides comprehensive metadata for LLM integration with clear field documentation.
75-112: LGTM! Message bus integration looks solid.The
bind(),refresh_tools(), andhandle_discover()methods correctly implement the messagebus lifecycle for tool discovery and registration.
114-134: LGTM! Robust message bus error handling.The broad exception catch in
handle_call()is appropriate for a message bus handler, ensuring errors are propagated without crashing the service. The error response format is clear and includes the toolbox identifier.
136-179: LGTM! Clean validation logic.The static validation methods properly leverage Pydantic's validation capabilities with appropriate error context. The separation of input and output validation improves clarity.
259-310: LGTM! Dynamic tool discovery and JSON schema generation work correctly.The
get_tool()method's refresh-on-miss behavior supports dynamic tool discovery for runtime-modifiable toolboxes. Thetool_json_listproperty correctly leverages Pydantic's schema generation for LLM integration. The abstractdiscover_tools()method provides a clear extension point for concrete implementations.Based on learnings
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
- Add Apache 2.0 license header (fixes License CI) - Fix discover_tools() result not stored in __init__ (tools dict was always empty after init) - Add LOG.debug on discover failure instead of silent except: pass - Remove unused `Field` import (pydantic) - docs: add docs/api/agent-tools.md — ToolBox plugin API reference - docs: update docs/index.md to link agent-tools.md - test: add test/unittests/test_agent_tools.py — 21 tests covering init, call_tool (dict/model/error paths), lazy refresh, bus handlers, json schema Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
- PluginTypes: add AGENT_TOOLBOX (opm.agents.toolbox) and AGENT_LOOP (opm.agents.loop) - PluginTypes: remove PERSONA_TOOL (opm.persona.tool) — wrong namespace for tool plugins - PluginConfigTypes: add AGENT_TOOLBOX and AGENT_LOOP config entries - persona.py: find_toolbox_plugins() now uses AGENT_TOOLBOX - agent_tools.py: update ToolBox docstring to reference correct entry point group - templates/agents.py: add AgenticLoopEngine(ChatEngine) — opm.agents.loop entry point - standard load_toolboxes(toolboxes) interface for persona-injected ToolBoxes - presents as ChatEngine to all callers; loop internals are implementation details - docs/api/agents.md: document AgenticLoopEngine and persona config toolboxes pattern - docs/api/agent-tools.md: fix entry point to opm.agents.toolbox Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
AgenticLoopEngine is now implemented in the standalone ovos-agentic-loop
repo (Agent Plugins/ovos-agentic-loop) and registers under the existing
opm.agents.chat entry-point group — no new OPM type is needed.
Removed:
- PluginTypes.AGENT_LOOP ("opm.agents.loop")
- PluginConfigTypes.AGENT_LOOP ("opm.agents.loop.config")
- AgenticLoopEngine class from templates/agents.py
- AgenticLoopEngine section from docs/api/agents.md
Co-Authored-By: Claude Sonnet 4.6 <[email protected]>
Greetings from the CI/CD pipeline! 🏗️I've aggregated the results of the automated checks for this PR below. 📋 Repo HealthHealth report: The repository is thriving! 🌟 ✅ All required files present. Latest Version: ✅ 🔍 LintThe results are in the bag! 🎒 ❌ ruff: issues found — see job log 🔒 Security (pip-audit)I've checked the vulnerability database for hits. 🎯 ✅ No known vulnerabilities found (59 packages scanned). 📊 CoverageChecking the weather: is it raining tests? ☔ ✅ 84.9% total coverage Files below 80% coverage (22 files)
Full report: download the 🔨 Build TestsEnsuring the foundation is solid for these changes. 🏛️ ✅ All versions pass
🏷️ Release PreviewI've checked the countdown clock for the next release. ⏰ Current:
✅ PR title follows conventional commit format. 🚀 Release Channel Compatibility Predicted next version:
⚖️ License CheckEnsuring the project remains 100% open source. 🔓 ✅ No license violations found (40 packages). License distribution: 10× MIT License, 6× Apache Software License, 6× MIT, 4× Apache-2.0, 2× BSD-3-Clause, 2× ISC License (ISCL), 2× PSF-2.0, 2× Python Software Foundation License, +6 more Full breakdown — 40 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. Stay curious and keep coding! 🚀 |
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 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/index.md`:
- Line 24: The index references the old entry-point name "opm.persona.tool"
which no longer matches the plugin type used elsewhere; update this documented
entry to use the new plugin type string (replace "opm.persona.tool" with the
exact plugin type identifier used across the PR) so authors register under the
correct group—look for the canonical plugin type string used in other
docs/manifests in the PR and mirror that here.
In `@ovos_plugin_manager/templates/agent_tools.py`:
- Around line 112-126: The discovery handler is serializing a potentially stale
tool cache; before building response_data in handle_discover call the toolbox
refresh method (discover_tools()) to update self.tool_json_list (and catch/log
exceptions so discovery still responds). Ensure the refreshed list is used when
constructing {"tools": self.tool_json_list, "toolbox_id": self.toolbox_id};
mirror the same refresh-on-miss behavior used by ToolBox.get_tool().
- Around line 165-170: The validation exceptions currently include and may
forward raw payloads (tool_kwargs / result) which can leak secrets; update
validate_input (where ArgsModel = tool.argument_schema and
ArgsModel(**tool_kwargs) is instantiated) and validate_output (the symmetric
block) to avoid embedding the full payload in the raised ValueError. Instead
raise a generic, non-echoing error message referencing tool.name and the
validation error type/message (e.g., str(e) or e.__class__.__name__) without
including tool_kwargs or the result; ensure handle_call continues to propagate
the error but no longer contains raw args/results in its text.
- Around line 143-144: The bus emission uses result.model_dump() which outputs
Python-mode serialization; update the emission so ToolOutput is serialized in
JSON mode to match tool_json_list's model_json_schema and handle
datetimes/UUIDs/enums/aliases correctly—locate the call to call_tool returning
ToolOutput and the bus.emit(message.response(...)) that references
result.model_dump(), and replace that serialization with JSON-mode Pydantic dump
(e.g., use model_dump(mode="json") or equivalent json-compatible dump) so the
emitted payload aligns with the advertised schema and toolbox_id.
In `@requirements/requirements.txt`:
- Line 8: The test extras in pyproject.toml are missing pydantic, causing CI to
fail when GitHub Actions installs with install_extras: 'test'; update the
[project.optional-dependencies] test group in pyproject.toml to include
"pydantic~=2.0" (i.e., add pydantic to the test list used by the test extras so
pytest can import pydantic during CI runs).
In `@test/unittests/test_agent_tools.py`:
- Around line 48-49: Remove the dead placeholder class that references
MathToolBox before its real definition: delete the stub "class
MathToolBox(MathToolBox if False else object): pass" so only the actual
MathToolBox class definition remains; this eliminates the F821 undefined-name
error while preserving the real MathToolBox implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4df8feed-69f4-4d33-aca7-139321f40366
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (8)
docs/api/agent-tools.mddocs/api/agents.mddocs/index.mdovos_plugin_manager/persona.pyovos_plugin_manager/templates/agent_tools.pyovos_plugin_manager/utils/__init__.pyrequirements/requirements.txttest/unittests/test_agent_tools.py
🚧 Files skipped from review as they are similar to previous changes (1)
- ovos_plugin_manager/persona.py
Addressed 6 critical issues from CodeRabbit review on tool plugins PR: **pyproject.toml:** - Added pydantic~=2.0 to test optional-dependencies Reason: GitHub Actions installs with 'test' extras from pyproject.toml; pytest was failing with ModuleNotFoundError for pydantic **test/unittests/test_agent_tools.py:** - Removed dead placeholder definition (line 48-49) Was causing Ruff F821 (Undefined name) warning **docs/index.md:** - Fixed entry-point name from opm.persona.tool to opm.agents.toolbox Matches actual implementation in ToolBox class definition **ovos_plugin_manager/templates/agent_tools.py:** 1. handle_discover() (line 123): Call refresh_tools() before broadcasting - Prevents stale cache if initial discover_tools() fails - Ensures dynamic tool discovery works for bus-only clients 2. handle_call() (line 144): Use model_dump(mode='json') for consistency - Matches model_json_schema() advertised in tool_json_list - Prevents divergence for datetime, UUID, enum, or aliased fields 3. validate_input() (line 170): Remove raw tool_kwargs from error message - Security fix: prevents echoing secrets/user content on bus 4. validate_output() (line 193): Remove raw result from error message - Security fix: prevents echoing secrets/user content on bus **Testing:** - All 1011 unit tests pass - All 21 agent_tools tests pass - Python syntax verified AI-Generated Change: - Model: Claude Haiku 4.5 - Intent: Address CodeRabbit PR review comments for tool plugins feature - Impact: 6 critical fixes for cache staleness, JSON serialization, security, and dependencies - Verified via: uv run pytest test/unittests/ -v (1011 passed)
- Added 3 new FAQ entries: Agent Tools API, dynamic discovery, JSON serialization - Added comprehensive MAINTENANCE_REPORT entry documenting all 6 fixes: * pyproject.toml dependency fix * Test placeholder removal * Documentation accuracy fix * Cache staleness prevention * JSON serialization consistency * Security fixes for error messages - Included AI transparency (model, actions, oversight notes) - Linked to source file locations for future reference AI-Generated Change: - Model: Claude Haiku 4.5 - Intent: Document PR #340 fixes and new agent tools features for future maintainers - Impact: Improved discoverability and traceability of changes - Verified via: Manual verification of FAQ clarity and MAINTENANCE_REPORT completeness
1 participant
docs: OpenVoiceOS/ovos-technical-manual#38
Summary by CodeRabbit
New Features
Public API
Documentation
Tests
Dependencies