Skip to content

fix: Tool watcher checks for tools repo even if initially missing #950

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix: Tool watcher checks for tools repo even if initially missing #950

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
76848e2
fix: Tool watcher checks for tools repo even if initially missing
Unshure Sep 29, 2025
280bd76
Update registry.py
Unshure Sep 30, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion src/strands/tools/registry.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -246,7 +246,11 @@ def get_tools_dirs(self) -> List[Path]:
tool_dirs.append(directory)
logger.debug("tools_dir=<%s> | found tools directory", directory)
else:
logger.debug("tools_dir=<%s> | tools directory not found", directory)
logger.debug(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any reason not to throw? I feel like if you enable this option and there is no directory, it's not obvious that it's not going to work.

I feel like because they explicitly opt-ed in, we should either throw or create the directory

Copy link
Member Author

@Unshure Unshure Sep 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Backwards compatibility ¯\_(ツ)_/¯

I actually mean to update this to a warning instead of debug, so at least this is more visible

"tools_dir=<%s> | tools directory not found."
"You must create a local tools directory to enable hot-reloading of tools",
directory,
)

return tool_dirs

Expand Down
4 changes: 2 additions & 2 deletions src/strands/tools/watcher.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,6 @@ def on_modified(self, event: Any) -> None:
if event.src_path.endswith(".py"):
tool_path = Path(event.src_path)
tool_name = tool_path.stem

if tool_name not in ["__init__"]:
# Delegate to all registered handlers for this directory
for handler in ToolWatcher._registry_handlers.get(self.dir_path, {}).values():
Expand Down Expand Up @@ -122,7 +121,8 @@ def start(self) -> None:
if dir_str not in ToolWatcher._watched_dirs:
# First time seeing this directory, create a master handler
master_handler = self.MasterChangeHandler(dir_str)
ToolWatcher._shared_observer.schedule(master_handler, dir_str, recursive=False)

ToolWatcher._shared_observer.schedule(master_handler, str(tools_dir), recursive=False)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, can you explain why this is fixing the bug? Is there some mutation happening?

Like like L111 has dir_str = str(tools_dir) so I'm not sure whats changing?

ToolWatcher._watched_dirs.add(dir_str)
logger.debug("tools_dir=<%s> | started watching tools directory", tools_dir)
else:
Expand Down
22 changes: 22 additions & 0 deletions tests/strands/tools/test_watcher.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
Tests for the SDK tool watcher module.
"""

from pathlib import Path
from unittest.mock import MagicMock, patch

import pytest
Expand Down Expand Up @@ -96,3 +97,24 @@ def test_on_modified_error_handling(mock_reload_tool):

# Verify that reload_tool was called
mock_reload_tool.assert_called_once_with("test_tool")


@patch("strands.tools.watcher.ToolWatcher._shared_observer")
@patch.object(ToolRegistry, "get_tools_dirs")
def test_start_uses_correct_path_for_observer_schedule(mock_get_tools_dirs, mock_observer):
"""Test that start method schedules observer with correct path (str(tools_dir) not dir_str)."""

# Mock the tools directories
tools_dir = Path("/test/tools")
mock_get_tools_dirs.return_value = [tools_dir]

tool_registry = ToolRegistry()
watcher = ToolWatcher(tool_registry)

# Call start method
watcher.start()

# Verify that schedule was called with str(tools_dir)
mock_observer.schedule.assert_called_once()
args = mock_observer.schedule.call_args[0]
assert args[1] == str(tools_dir) # Second argument should be the string path
Loading