Proposal: Simplify `ToolDefinition` Architecture

[xingyaoww]

Problem Statement

The current tool system has multiple overlapping concepts (Tool, ToolDefinition, ToolBase) that make the architecture confusing and inconsistent:

1. Inconsistent tool definition patterns: Tools are defined in two different ways:

   • Direct instances: FinishTool = ToolDefinition(...) with executor already initialized
   • Subclasses: class BashTool(ToolDefinition): ... with custom .create() method

2. Ambiguous .create() contract:

   • Direct instance tools (like FinishTool, ThinkTool) have a .create() method that raises NotImplementedError
   • Subclass tools (like BashTool, FileEditorTool) implement .create() to initialize the executor
   • This makes the contract unclear and forces defensive programming

3. Confusion about naming:

   • Tool (spec.py): Serializable spec with name and params
   • ToolDefinition: Concrete tool that can be executed
   • ToolBase: Abstract base class for tools
   • The relationship and purpose of each is not immediately clear

Current Architecture

# spec.py - Serializable spec passed to Agent
class Tool(BaseModel):
    name: str
    params: dict[str, Any]

# tool.py - Abstract base
class ToolBase[ActionT, ObservationT](ABC):
    name: str
    description: str
    action_type: type[Action]
    observation_type: type[Observation] | None
    executor: ToolExecutor | None  # Optional!
    
    @abstractmethod
    def create(cls, *args, **kwargs) -> Sequence[Self]:
        ...

# tool.py - Concrete implementation
class ToolDefinition[ActionT, ObservationT](ToolBase):
    @classmethod
    def create(cls, *args, **kwargs) -> Sequence[Self]:
        raise NotImplementedError(...)  # Hacky!

Usage Pattern 1 (Direct Instance):

# builtins/finish.py
FinishTool = ToolDefinition(
    name="finish",
    action_type=FinishAction,
    observation_type=FinishObservation,
    description=TOOL_DESCRIPTION,
    executor=FinishExecutor(),  # Already initialized
)

Usage Pattern 2 (Subclass):

# tools/execute_bash/definition.py
class BashTool(ToolDefinition[ExecuteBashAction, ExecuteBashObservation]):
    @classmethod
    def create(cls, conv_state, **params) -> Sequence["BashTool"]:
        executor = BashExecutor(working_dir=conv_state.workspace.working_dir, ...)
        return [cls(
            name="execute_bash",
            action_type=ExecuteBashAction,
            observation_type=ExecuteBashObservation,
            description=TOOL_DESCRIPTION,
            executor=executor,  # Initialized in create()
        )]

Constraints to Satisfy

1. Serializable and lazy: Tools passed to Agent class must be serializable (for remote conversation) and lazy (should not materialize until agent is initialized)

   • Currently satisfied by Tool (spec) with name and params

2. Two-level architecture: We need at least two things:

   • A "spec" version of the tool that's serializable (Tool)
   • Actual tool instance that can be executed (ToolDefinition)

3. Flexible executor initialization: Ability to decide whether to provide tool executor during tool initialization (e.g., for sharing executors among multiple tool instances)

Proposed Solution

Make all tools follow the subclass pattern with a clear contract:

1. All tools should be subclasses of ToolDefinition, not direct instances
2. Make .create() an @abstractmethod in ToolBase so the contract is explicit
3. Each tool class implements .create() which returns instances with executor populated
4. For simple built-in tools, create minimal subclasses instead of direct instances

Example Refactoring

Before (Direct Instance):

FinishTool = ToolDefinition(
    name="finish",
    action_type=FinishAction,
    observation_type=FinishObservation,
    description=TOOL_DESCRIPTION,
    executor=FinishExecutor(),
)

After (Subclass):

class FinishTool(ToolDefinition[FinishAction, FinishObservation]):
    @classmethod
    def create(cls, conv_state=None, **params) -> Sequence["FinishTool"]:
        if params:
            raise ValueError("FinishTool doesn't accept parameters")
        return [cls(
            name="finish",
            action_type=FinishAction,
            observation_type=FinishObservation,
            description=TOOL_DESCRIPTION,
            executor=FinishExecutor(),
            annotations=ToolAnnotations(...),
        )]

Benefits

1. Predictable pattern: All tools follow the same subclass pattern
2. Clear contract: .create() is @abstractmethod, making the interface explicit
3. Consistent naming: Tool classes have reliable names (e.g., BashTool.name)
4. Better for registration: Registry can reference tools by class name without instantiating
5. Type safety: Eliminates the NotImplementedError hack
6. Maintains flexibility: Executor can still be initialized in .create() with custom parameters

Alternative Considered

Option B: Lazy initialization with futures/proxies

Figure out some hacks for ToolDefinition.create() to return a serializable "future" that materializes on demand:

# Hypothetical proxy pattern
class ToolProxy(BaseModel):
    _tool_class: type[ToolDefinition]
    _params: dict[str, Any]
    
    def materialize(self, conv_state) -> ToolDefinition:
        return self._tool_class.create(conv_state, **self._params)[0]

Why we prefer Option A (subclass pattern):

• Simpler and more straightforward
• No magic or proxy objects
• Existing registry pattern already handles lazy initialization
• Better aligns with Python idioms

Implementation Plan

1. Refactor built-in tools (FinishTool, ThinkTool) to be subclasses
2. Make ToolBase.create() an @abstractmethod
3. Remove the NotImplementedError in ToolDefinition.create()
4. Update tests to verify all tools follow the pattern
5. Update documentation to clarify the architecture

Open Questions

1. Should we rename ToolDefinition to something clearer (e.g., ToolClass, ToolFactory)?
2. Should we consider making executor non-optional in ToolDefinition (with a separate "ToolSpec" for serialization)?
3. How do we handle backward compatibility for users who may have created direct ToolDefinition instances?

Related

• Original discussion: Implement automatic tool registration on import #862
• TODO comment in code: Improve tools implementation #493

[xingyaoww]

@OpenHands let's implement the proposed method first

[openhands-ai]

I'm on it! xingyaoww can track my progress at all-hands.dev

[openhands-ai]

Summary

I have successfully implemented the proposed solution from issue #970 to simplify and standardize the ToolDefinition architecture.

✅ Changes Completed

1. Refactored Built-in Tools to Subclass Pattern

   • FinishTool: Converted from direct ToolDefinition instance to a proper subclass with .create() method
   • ThinkTool: Converted from direct ToolDefinition instance to a proper subclass with .create() method
   • Both now follow the same consistent pattern as BashTool and other complex tools

2. Enforced Abstract Contract

   • Made ToolBase.create() an @abstractmethod to enforce clear interface contract
   • Kept ToolDefinition.create() with NotImplementedError for backward compatibility
   • All tool classes must now implement the .create() method

3. Updated Agent Integration

   • Modified base.py to call .create() on built-in tool classes to get instances
   • Minor cleanup in agent.py to use string literal for 'finish' tool

4. Fixed All Tests

   • Updated test_tool_serialization.py to use ToolBase.model_validate_json() for proper polymorphic deserialization
   • Updated test_builtins.py to call .create() on tool classes in BUILT_IN_TOOLS
   • Changed all Pydantic model type hints from ToolDefinition to ToolBase for polymorphism

✅ Testing

All 1039 tests pass successfully!

✅ Benefits Achieved

• Predictable pattern: All tools now follow the same subclass pattern
• Clear contract: .create() is @abstractmethod, making the interface explicit
• Type safety: Eliminates the NotImplementedError hack for built-in tools
• Better for registration: Registry can reference tools by class name without instantiation
• Maintains flexibility: Executor initialization still happens in .create() with custom parameters

📦 Pull Request

PR #971 created (draft): #971

The PR is ready for review and includes:

• Clear description of all changes
• Benefits and backward compatibility notes
• All tests passing confirmation

Branch: openhands/simplify-tool-definition-architecture

_{View full conversation}