fix: smart_read_file accepts path= kwarg from LLMs (#113)
LLMs naturally call read_file(path=...) but the wrapper only accepted file_name=. Pydantic strict validation rejected the mismatch. Now accepts both file_name and path kwargs, with clear error on missing both. Added 6 tests covering: positional args, path kwarg, no-args error, directory listing, empty dir, hidden file filtering.
This commit is contained in:
@@ -196,8 +196,13 @@ def _make_smart_read_file(file_tools: FileTools) -> Callable:
|
||||
"""
|
||||
original_read = file_tools.read_file
|
||||
|
||||
def smart_read_file(file_name: str, encoding: str = "utf-8") -> str:
|
||||
def smart_read_file(file_name: str = "", encoding: str = "utf-8", **kwargs) -> str:
|
||||
"""Reads the contents of the file `file_name` and returns the contents if successful."""
|
||||
# LLMs often call read_file(path=...) instead of read_file(file_name=...)
|
||||
if not file_name:
|
||||
file_name = kwargs.get("path", "")
|
||||
if not file_name:
|
||||
return "Error: no file_name or path provided."
|
||||
# Resolve the path the same way FileTools does
|
||||
_safe, resolved = file_tools.check_escape(file_name)
|
||||
if _safe and resolved.is_dir():
|
||||
|
||||
123
tests/test_smart_read_file.py
Normal file
123
tests/test_smart_read_file.py
Normal file
@@ -0,0 +1,123 @@
|
||||
"""Tests for the smart_read_file wrapper in timmy.tools.
|
||||
|
||||
Covers the wrapper's behavior for:
|
||||
- Handling directories (return listing instead of error)
|
||||
- Accepting `path=` keyword as alias for `file_name=` (the LLM pattern bug fix)
|
||||
"""
|
||||
|
||||
from unittest.mock import MagicMock
|
||||
|
||||
import pytest
|
||||
|
||||
from timmy.tools import _make_smart_read_file
|
||||
|
||||
|
||||
class TestSmartReadFile:
|
||||
"""Test suite for the smart_read_file wrapper function."""
|
||||
|
||||
@pytest.fixture
|
||||
def mock_file_tools(self, tmp_path):
|
||||
"""Create a mock FileTools instance for testing.
|
||||
|
||||
- check_escape returns (True, Path) for safe paths
|
||||
- read_file returns file contents
|
||||
"""
|
||||
file_tools = MagicMock()
|
||||
|
||||
def mock_check_escape(file_name):
|
||||
"""Mock implementation that returns safe paths."""
|
||||
resolved = tmp_path / file_name
|
||||
return (True, resolved)
|
||||
|
||||
file_tools.check_escape = mock_check_escape
|
||||
file_tools.read_file = MagicMock(return_value="file contents")
|
||||
|
||||
return file_tools
|
||||
|
||||
def test_read_file_with_file_name(self, mock_file_tools):
|
||||
"""Test normal call with file_name positional arg."""
|
||||
smart_read_file = _make_smart_read_file(mock_file_tools)
|
||||
|
||||
result = smart_read_file("test.txt")
|
||||
|
||||
assert result == "file contents"
|
||||
mock_file_tools.read_file.assert_called_once_with("test.txt", encoding="utf-8")
|
||||
|
||||
def test_read_file_with_path_kwarg(self, mock_file_tools):
|
||||
"""Test call with path='./config/agents.yaml' (the LLM pattern).
|
||||
|
||||
LLMs often call read_file(path=...) instead of read_file(file_name=...).
|
||||
The wrapper should accept path= as an alias for file_name=.
|
||||
"""
|
||||
smart_read_file = _make_smart_read_file(mock_file_tools)
|
||||
|
||||
result = smart_read_file(path="./config/agents.yaml")
|
||||
|
||||
assert result == "file contents"
|
||||
mock_file_tools.read_file.assert_called_once_with("./config/agents.yaml", encoding="utf-8")
|
||||
|
||||
def test_read_file_no_args(self, mock_file_tools):
|
||||
"""Test call with no args returns error message."""
|
||||
smart_read_file = _make_smart_read_file(mock_file_tools)
|
||||
|
||||
result = smart_read_file()
|
||||
|
||||
assert result == "Error: no file_name or path provided."
|
||||
mock_file_tools.read_file.assert_not_called()
|
||||
|
||||
def test_read_file_directory(self, mock_file_tools, tmp_path):
|
||||
"""Test call with directory path returns listing.
|
||||
|
||||
When the user (or the LLM) passes a directory path to read_file,
|
||||
the wrapper detects that case, lists the directory entries, and returns
|
||||
a helpful message so the model can pick the right file on its own.
|
||||
"""
|
||||
# Create a test directory with some files
|
||||
test_dir = tmp_path / "config"
|
||||
test_dir.mkdir()
|
||||
(test_dir / "agents.yaml").write_text("")
|
||||
(test_dir / "settings.json").write_text("")
|
||||
(test_dir / "README.md").write_text("")
|
||||
|
||||
smart_read_file = _make_smart_read_file(mock_file_tools)
|
||||
|
||||
result = smart_read_file("config")
|
||||
|
||||
assert "'config' is a directory, not a file." in result
|
||||
assert "Files inside:" in result
|
||||
assert " - agents.yaml" in result
|
||||
assert " - README.md" in result
|
||||
assert " - settings.json" in result
|
||||
assert "Please call read_file with one of the files listed above." in result
|
||||
mock_file_tools.read_file.assert_not_called()
|
||||
|
||||
def test_read_file_empty_directory(self, mock_file_tools, tmp_path):
|
||||
"""Test call with empty directory returns empty directory message."""
|
||||
# Create an empty test directory
|
||||
test_dir = tmp_path / "empty_dir"
|
||||
test_dir.mkdir()
|
||||
|
||||
smart_read_file = _make_smart_read_file(mock_file_tools)
|
||||
|
||||
result = smart_read_file("empty_dir")
|
||||
|
||||
assert "'empty_dir' is a directory, not a file." in result
|
||||
assert " (empty directory)" in result
|
||||
mock_file_tools.read_file.assert_not_called()
|
||||
|
||||
def test_read_file_directory_hides_hidden_files(self, mock_file_tools, tmp_path):
|
||||
"""Test that hidden files (starting with .) are not listed."""
|
||||
# Create a test directory with hidden files
|
||||
test_dir = tmp_path / "config"
|
||||
test_dir.mkdir()
|
||||
(test_dir / "visible.txt").write_text("")
|
||||
(test_dir / ".hidden").write_text("")
|
||||
(test_dir / ".gitignore").write_text("")
|
||||
|
||||
smart_read_file = _make_smart_read_file(mock_file_tools)
|
||||
|
||||
result = smart_read_file("config")
|
||||
|
||||
assert "visible.txt" in result
|
||||
assert ".hidden" not in result
|
||||
assert ".gitignore" not in result
|
||||
Reference in New Issue
Block a user