refactor: update tool registry interface #468

yakshpradippatel · 2025-07-15T14:49:23Z

Description

[DO NOT MERGE SINCE THIS IS A BACKWARDS INCOMPATIBLE CHANGE]

New CRUDL interface for Tool Registry for customers to directly use in their codebase

create_tool(tool)
read_tool(tool_name)
update_tool(tool)
delete_tool(tool_name)
list_tools()

Related Issues

https://github.com/strands-agents/private-sdk-python-staging/issues/26

Documentation PR

N/A

Type of Change

Breaking change to the ToolRegistry interface

Testing

I ran hatch run prepare

Checklist

I have read the CONTRIBUTING document
I have added any necessary tests that prove my fix is effective or my feature works
I have updated the documentation accordingly
I have added an appropriate example to the documentation to outline the feature, or no new docs are needed
My changes generate no new warnings
[] Any dependent changes have been merged and published

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Unshure · 2025-07-15T15:07:00Z

src/strands/tools/registry.py

+                except Exception as e:
+                    logger.warning("tool_name=<%s> | failed to create function tool | %s", name, e)
+
+        return tools

    def _update_tool_config(self, tool_config: Dict[str, Any], new_tool: NewToolDict) -> None:


Why do we need NewToolDict here? Can we just pass in ToolSpec which is already defined?

Unshure · 2025-07-15T15:09:59Z

src/strands/tools/registry.py

+        providing a clean single interface for all tool registration needs. This replaces
+        the previous multi-step pattern for better separation of concerns.


Lets not mention what this replaces

Unshure · 2025-07-15T15:11:27Z

src/strands/tools/registry.py

+                - Dictionaries with name/path keys
+                - Instance of an AgentTool
+            load_tools_from_directory: Whether to automatically discover and load tools
+                                     from the ./tools/ directory.


nit: Indents seem a little off here

Unshure · 2025-07-15T15:30:18Z

src/strands/tools/registry.py

+        logger.debug("tool_modules=<%s> | discovered", list(tool_modules.keys()))
+        return tool_modules
+
+    def _initialize_tools(self, load_tools_from_directory: bool = False) -> None:


Can we rename this to something like _load_tools_from_directory? And then instead of passing in the boolean, we conditionally call the method?

Unshure · 2025-07-15T15:32:21Z

src/strands/tools/registry.py

        self.registry: Dict[str, AgentTool] = {}
        self.dynamic_tools: Dict[str, AgentTool] = {}
        self.tool_config: Optional[Dict[str, Any]] = None

+    def register_tools(


What methods in this class are meant to be public or private? If the only public method is register_tools, can we put an _ before the method names of the others?

Unshure · 2025-07-15T15:32:46Z

src/strands/tools/registry.py

        self.registry: Dict[str, AgentTool] = {}
        self.dynamic_tools: Dict[str, AgentTool] = {}
        self.tool_config: Optional[Dict[str, Any]] = None


Are these attributes public or private? If private, and we put an _ prefix on them?

Unshure · 2025-07-15T15:35:18Z

src/strands/tools/registry.py

+            except Exception:
+                # Restore original state on any failure (atomic operation)
+                self.registry = original_registry
+                self.dynamic_tools = original_dynamic_tools
+                raise


Do we need to restore the tools here? If there is an unknown exception, it wont really matter if the tools are restored or not right? This should also simplify the logic in this function.

pgrayy · 2025-07-18T16:30:07Z

src/strands/agent/agent.py

@@ -285,15 +284,18 @@ def __init__(
        self.record_direct_tool_call = record_direct_tool_call
        self.load_tools_from_directory = load_tools_from_directory


It looks like we don't use this outside of init anymore so I think we can go ahead and remove it as a member variable.

pgrayy · 2025-07-18T16:44:29Z

src/strands/tools/registry.py

-            else:
-                logger.warning("tool=<%s> | unrecognized tool specification", tool)
+        # Process the tool - this will validate and register the tool
+        created_tool_name = self._process_tool(tool, validate_unique=True)


It looks like we are checking if tool name is unique multiple times. We should consolidate this logic.

_process_tool is extracting the name with similar logic to _extract_tool_name. We should try to remove the duplication.

pgrayy · 2025-07-18T16:56:33Z

src/strands/tools/registry.py

+        # Update tool configuration
+        if created_tool_name in self.agent_tools:
+            tool_spec = self.agent_tools[created_tool_name].tool_spec
+            self._update_tool_config(self._tool_config, tool_spec)


What is the purpose of _tool_config? I only see us adding to it and deleting from it. I'm not seeing where we actually use it.

pgrayy · 2025-07-18T16:59:09Z

src/strands/tools/registry.py

+            spec = normalize_tool_spec(spec)
+            self._validate_tool_spec(spec)


Do we need to normalize and validate the tool spec on every read? For efficiency, is this something that can be done upon creating or updating the tool?

pgrayy · 2025-07-18T17:01:42Z

src/strands/tools/registry.py

+            return tool_name
+        except Exception as e:
+            # Restore the original tool if the update fails
+            if original_tool:


Nit: We shouldn't need this condition. original_tool should be guaranteed to exist.

pgrayy · 2025-07-18T17:02:40Z

src/strands/tools/registry.py

+        Raises:
+            ValueError: If the specified tool doesn't exist.
+        """
+        if tool_name is None:


Nit: We shouldn't need this condition since we are using type hints.

pgrayy · 2025-07-18T17:05:36Z

src/strands/tools/registry.py

+
+        # Make a backup of the tool in case we need to restore it
+        original_tool = self.agent_tools.get(tool_name)
+        original_hot_reload = self._hot_reload_tools.get(tool_name)


What is the difference between original_tool and original_hot_reload? Also, how is _hot_reload_tools used? I only see us adding to it and deleting from it. I don't see us using it though.

pgrayy · 2025-07-18T17:06:58Z

src/strands/tools/registry.py

+                spec = normalize_tool_spec(spec)
+                self._validate_tool_spec(spec)


Same question as in read_tool, do we need to normalize and validate here?

pgrayy · 2025-07-18T17:24:34Z

Do you think there is a way to simplify the tool registry? I see we are adding 200 more lines of code and I am worried it's going to become a bit more challenging to maintain. Just looking at some of the method names, it seems like their might be some redundancy we can eliminate. For example, I see we have the following:

_find_tool_directories
_load_tool_from_filepath
_discover_tool_modules
_load_directory_tools

cagataycali · 2025-07-18T18:06:23Z

src/strands/agent/agent.py

@@ -285,15 +284,18 @@ def __init__(
        self.record_direct_tool_call = record_direct_tool_call
        self.load_tools_from_directory = load_tools_from_directory

-        self.tool_registry = ToolRegistry()
+        # Initialize the tool registry with the directory loading option
+        self.tool_registry = ToolRegistry(load_tools_from_directory=self.load_tools_from_directory)


Since we're changing this interface can we also allow the directories to be passed for watching ^^

Idea is we're only allowing ./tools directory to be loaded but customers can pass their directories like:

Agent(tool_directories=["./tools", -> default, ...]

cagataycali · 2025-07-18T18:07:17Z

src/strands/agent/agent.py

-        if tools is not None:
-            self.tool_registry.process_tools(tools)
+        # Initialize individual tools if provided
+        for tool in tools or []:


nit: for tool in tools or []: do we need or [] here ^^ 🤔

cagataycali · 2025-07-18T18:08:31Z

src/strands/tools/registry.py

-This module provides the central registry for all tools available to the agent, including discovery, validation, and
-invocation capabilities.
+This module provides a central registry for managing agent tools, including discovery,
+validation, registration, and invocation capabilities.


Can we add detailed docstrings here for agents, so they can immediately understand how to use the registry 🤔 💭

cagataycali · 2025-07-18T18:13:24Z

src/strands/tools/registry.py

+                updated_tools.append(tool_name)
+            except Exception as e:
+                logger.warning("tool_name=<%s> | failed to update from directory | %s", tool_name, e)
+                # Fail atomically - if any tool fails to update, the entire operation fails


I believe if one tool failed to update we can skip that tool to be loaded instead of raising a ValueError ^^

cagataycali · 2025-07-18T18:13:52Z

src/strands/tools/registry.py

+            A list of Path objects for current working directory's "./tools/".
+        """
+        # Current working directory's tools directory
+        cwd_tools_dir = Path.cwd() / "tools"


Can we allow people to set the tools directories from Agent interface?

zastrowm · 2025-07-21T14:18:22Z

src/strands/tools/registry.py

@@ -24,244 +24,318 @@


 class ToolRegistry:
-    """Central registry for all tools available to the agent.
+    """Agent Tool CRUD Interface.


I think the "Central registry for all tools available to the agent." hits closer to the mark TBH. As someone reading through the code, the next sentence tells me the same thing so I'd rather see something like:

Contains the collection of tools available to the agent.

zastrowm · 2025-07-21T14:19:49Z

src/strands/tools/registry.py

+        if load_tools_from_directory:
+            self._load_directory_tools()
+
+    def create_tool(self, tool: Union[str, dict[str, str], Any]) -> str:


I'm not sure that create makes sense here. In some cases, it's wrapping an instance of an AgentTool, in other cases it's adding it directly. IMHO, here it makes more sense that this would be called add_tool or simply add

zastrowm · 2025-07-21T14:20:29Z

src/strands/tools/registry.py

+        if load_tools_from_directory:
+            self._load_directory_tools()
+
+    def create_tool(self, tool: Union[str, dict[str, str], Any]) -> str:


Any reason not to return the wrapped tool instance AgentTool vs the name of the tool?

zastrowm · 2025-07-21T14:21:33Z

src/strands/tools/registry.py

+
+        return created_tool_name
+
+    def read_tool(self, tool_name: str) -> ToolSpec:


Per the description, this seems more like read_tool_spec instead of read_tool

zastrowm · 2025-07-21T14:22:40Z

src/strands/tools/registry.py

+        try:
+            # Update the tool
+            self._update_single_tool(tool, validate_unique=False)
+            return tool_name


I'd rather see us returning the AgentTool which already has the name attached AFAIK

zastrowm · 2025-07-21T14:23:23Z

src/strands/tools/registry.py

+            logger.warning("tool_name=<%s> | failed to update tool | %s", tool_name, e)
+            raise ValueError(f"Failed to update tool '{tool_name}': {str(e)}") from e
+
+    def delete_tool(self, tool_name: str) -> str:


Given that this class is named ToolRegistry perhaps we can name this as just delete? Same with the other {verb}_{noun} names?

zastrowm · 2025-07-21T14:24:31Z

src/strands/tools/registry.py

-        # Register in dynamic tools if applicable
-        if tool.is_dynamic:
-            self.dynamic_tools[tool.tool_name] = tool
+    def list_tools(self) -> Dict[str, ToolSpec]:


It's a little weird that a list method is returning a dictionary. Would this be better named as get_tool_specs or something?

yakshpradippatel temporarily deployed to auto-approve July 15, 2025 14:49 — with GitHub Actions Inactive

Unshure reviewed Jul 15, 2025

View reviewed changes

yakshpradippatel force-pushed the tool-registry branch from d523d6e to bfbd518 Compare July 16, 2025 10:11

yakshpradippatel temporarily deployed to auto-approve July 16, 2025 10:11 — with GitHub Actions Inactive

yakshpradippatel mentioned this pull request Jul 18, 2025

refactor: update tools and tools test to call new registry CRUDL interface strands-agents/tools#153

Open

8 tasks

yakshpradippatel force-pushed the tool-registry branch from bfbd518 to 2033bbd Compare July 18, 2025 01:47

yakshpradippatel temporarily deployed to auto-approve July 18, 2025 01:48 — with GitHub Actions Inactive

pgrayy reviewed Jul 18, 2025

View reviewed changes

cagataycali reviewed Jul 18, 2025

View reviewed changes

zastrowm requested changes Jul 21, 2025

View reviewed changes

cagataycali mentioned this pull request Jul 22, 2025

Automatically flatten nested tool collections #508

Merged

7 tasks

refactor: update tool registry interface

30a4d42

yakshpradippatel force-pushed the tool-registry branch from 2033bbd to 30a4d42 Compare July 25, 2025 15:13

yakshpradippatel had a problem deploying to auto-approve July 25, 2025 15:13 — with GitHub Actions Failure

		providing a clean single interface for all tool registration needs. This replaces
		the previous multi-step pattern for better separation of concerns.

		@@ -285,15 +284,18 @@ def __init__(
		self.record_direct_tool_call = record_direct_tool_call
		self.load_tools_from_directory = load_tools_from_directory

		spec = normalize_tool_spec(spec)
		self._validate_tool_spec(spec)


		return created_tool_name

		def read_tool(self, tool_name: str) -> ToolSpec:

refactor: update tool registry interface #468

Are you sure you want to change the base?

refactor: update tool registry interface #468

Conversation

yakshpradippatel commented Jul 15, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Documentation PR

Type of Change

Testing

Checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pgrayy commented Jul 18, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

yakshpradippatel commented Jul 15, 2025 •

edited

Loading