Conversation
WalkthroughThis pull request adds comprehensive documentation for the OpenVoiceOS Agent Tools framework, including core concepts (AgentTool, ToolBox), consumption methods (Direct Python and MessageBus-based), JSON Schema representations, and implementation examples. The documentation is registered in the mkdocs navigation. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
docs/tools.md(1 hunks)mkdocs.yml(1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/tools.md
[grammar] ~37-~37: Ensure spelling is correct
Context: ...s. * Bus Integration: Registering messagebus handlers to support dynamic discovery a...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
[grammar] ~52-~52: Ensure spelling is correct
Context: ...nefits:** * Direct Access: No messagebus latency. * Strong Validation:...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🪛 markdownlint-cli2 (0.18.1)
docs/tools.md
27-27: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
28-28: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
36-36: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
37-37: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
38-38: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
50-50: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
51-51: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
52-52: Unordered list indentation
Expected: 2; Actual: 6
(MD007, ul-indent)
53-53: Unordered list indentation
Expected: 2; Actual: 6
(MD007, ul-indent)
54-54: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
67-67: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
71-71: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
72-72: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
92-92: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
102-102: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
103-103: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
113-113: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
🔇 Additional comments (1)
mkdocs.yml (1)
48-48: Navigation entry correctly added.The new navigation entry is properly formatted and logically placed under the AI Agents section alongside related documentation.
| * **`ToolArguments(BaseModel)`**: Tool consumers (agents) must format their input according to the concrete Pydantic model derived from this base. This ensures rigorous input validation. | ||
| * **`ToolOutput(BaseModel)`**: The result of a tool call is guaranteed to conform to the concrete Pydantic model derived from this base. This ensures reliable data handling by the consuming agent. |
There was a problem hiding this comment.
Fix unordered list indentation throughout the document (MD007).
Markdown linting will fail due to inconsistent list indentation. Unordered lists should not have leading spaces; they should start at column 0.
Apply these diffs to fix the indentation:
#### 2. ToolArguments and ToolOutput
These are base classes for Pydantic models that define the data contract for a tool.
- * **`ToolArguments(BaseModel)`**: Tool consumers (agents) must format their input according to the concrete Pydantic model derived from this base. This ensures rigorous input validation.
- * **`ToolOutput(BaseModel)`**: The result of a tool call is guaranteed to conform to the concrete Pydantic model derived from this base. This ensures reliable data handling by the consuming agent.
+* **`ToolArguments(BaseModel)`**: Tool consumers (agents) must format their input according to the concrete Pydantic model derived from this base. This ensures rigorous input validation.
+* **`ToolOutput(BaseModel)`**: The result of a tool call is guaranteed to conform to the concrete Pydantic model derived from this base. This ensures reliable data handling by the consuming agent.The `ToolBox` handles:
- * **Tool Discovery:** Implementing the abstract `discover_tools()` method to define the available tools.
- * **Bus Integration:** Registering messagebus handlers to support dynamic discovery and remote calling (`ovos.persona.tools.discover`, `ovos.persona.tools.<toolbox_id>.call`).
- * **Execution and Validation:** Providing the `call_tool(name, tool_kwargs)` method, which is the official interface for executing tools, including mandatory **input and output Pydantic validation**.
+* **Tool Discovery:** Implementing the abstract `discover_tools()` method to define the available tools.
+* **Bus Integration:** Registering message bus handlers to support dynamic discovery and remote calling (`ovos.persona.tools.discover`, `ovos.persona.tools.<toolbox_id>.call`).
+* **Execution and Validation:** Providing the `call_tool(name, tool_kwargs)` method, which is the official interface for executing tools, including mandatory **input and output Pydantic validation**.Note: Apply similar fixes to all remaining indentation errors throughout the file (lines 50–53, 54, 67, 71–72, 92, 102–103, 113).
Also applies to: 36-38
🧰 Tools
🪛 markdownlint-cli2 (0.18.1)
27-27: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
28-28: Unordered list indentation
Expected: 0; Actual: 2
(MD007, ul-indent)
🤖 Prompt for AI Agents
In docs/tools.md around lines 27-28 (and also apply to 36-38, 50-53, 54, 67,
71-72, 92, 102-103, 113), the unordered list items have leading spaces causing
MD007 lint errors; remove the leading spaces so each unordered list marker (*)
starts at column 0, and normalize indentation for nested list items to use
consistent two-space indents (or none if top-level) throughout the file to fix
all similar occurrences.
mikejgray
left a comment
There was a problem hiding this comment.
Some comments, but overall I think this documentation is very easy to read and understand. It makes it trivial to implement and I'm looking forward to trying some of these out!
|
|
||
| | Attribute | Type | Description | | ||
| | :--- | :--- | :--- | | ||
| | **`name`** | `str` | The **unique**, snake_case identifier for the tool. This name is typically used by the agent/LLM to reference the function. | |
There was a problem hiding this comment.
Is snake_case strictly enforced or just suggested?
There was a problem hiding this comment.
just seems to be how everyone is doing it, but there's no specific checks to enforce it
| | Attribute | Type | Description | | ||
| | :--- | :--- | :--- | | ||
| | **`name`** | `str` | The **unique**, snake_case identifier for the tool. This name is typically used by the agent/LLM to reference the function. | | ||
| | **`description`** | `str` | A detailed, natural language explanation of the tool's purpose and how it should be used. Essential for LLM reasoning. | |
There was a problem hiding this comment.
Guidelines around length would be helpful here. A character/word limit or suggested minimum would make it easier to determine how to fulfill this
| | **`argument_schema`** | `Type[ToolArguments]` | A Pydantic model defining the **required structure and types** for the input arguments. | | ||
| | **`output_schema`** | `Type[ToolOutput]` | A Pydantic model defining the **guaranteed structure and types** for the tool's output. | |
There was a problem hiding this comment.
Is there a hard requirement for Pydantic? I personally prefer it for typing in Python, but I know it's not the go-to for all projects (Home Assistant comes to mind)
There was a problem hiding this comment.
pydantic is being used to validate the inputs and outputs to the tools, i like it because it gives very clear errors when input/output is malformed
|
|
||
| These are base classes for Pydantic models that define the data contract for a tool. | ||
|
|
||
| * **`ToolArguments(BaseModel)`**: Tool consumers (agents) must format their input according to the concrete Pydantic model derived from this base. This ensures rigorous input validation. |
There was a problem hiding this comment.
Have you tested if LLMs can handle Pydantic as a contract well? I know it's not just for LLMs but it will probably be a popular use case
There was a problem hiding this comment.
this will probably depend on the LLM, LLMs may output garbage or invalid data so thats why pydantic is important, it validates that the LLM followed the prompt and gave results in right format.
this is up to the agent developer to handle, all the toolbox does is throw an error then the agent needs to either retry or do somehing else
| * **Strong Validation:** Enforces the `argument_schema` and `output_schema` before and after execution, ensuring data integrity. | ||
| * **Usage Flow (Internal to Agent):** | ||
| 1. Get the `ToolBox` instance (e.g., via a plugin loading utility). | ||
| 2. Call: `validated_output = toolbox.call_tool(name="tool_name", tool_kwargs={"arg1": value})` |
There was a problem hiding this comment.
It will probably make more sense when I look at code, but I thought you pass a tool a Pydantic model?
There was a problem hiding this comment.
the pydantic model is part of the tool definition, not really user facing, the pydantic validation happens inside the toolbox.call_tool method
tool users should not need to worry about pydantic models, just tool developers
| "type": "ovos.persona.tools.<toolbox_id>.call.response", | ||
| "data": { | ||
| "toolbox_id": "web_search_tools", | ||
| "error": "ValueError: Invalid input for 'tool_name'..." |
There was a problem hiding this comment.
ToolOutput is supposed to be the guaranteed response per the tool contract, but this would be a different format, wouldn't it? Is there going to be something like a ToolError class and a tool could return ToolOutput | ToolError?
There was a problem hiding this comment.
this is not the tool output itself, this is the messagebus handler, when an exception happened using the tool, so in this case there's no tool output because it failed
| class FileWriteOutput(ToolOutput): | ||
| """Output structure for the write_file tool.""" | ||
| path: str = Field(description="The path to the file that was written.") | ||
| bytes_written: int = Field(description="The number of bytes successfully written.") | ||
| success: bool = Field(default=True) | ||
| error: Optional[str] = None |
There was a problem hiding this comment.
Related to my question above, seems that there are several ways to go about handling valid outputs vs. errors. Might be good to standardize on one for consistency
| except RuntimeError as e: | ||
| print(f"\nExecution Failed (Tool Logic/Output Validation): {e}") | ||
|
|
||
| # solve_file_problem() |
There was a problem hiding this comment.
Is this meant to be commented out for this example?
| - Personas: 150-personas.md | ||
| - AI Transformers: 151-llm-transformers.md | ||
| - Specialized Solvers: 150-advanced_solvers.md | ||
| - Tools for Agents: tools.md |
There was a problem hiding this comment.
This should have a number to conform to the rest of the files
companion to OpenVoiceOS/ovos-plugin-manager#340
Summary by CodeRabbit