Skip to content

fix(core): return clear error when file read tool receives a directory path #9090

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

Closed
THE-FLIER wants to merge 1 commit into from
Closed

fix(core): return clear error when file read tool receives a directory path #9090

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
34 changes: 29 additions & 5 deletions astrbot/core/computer/file_read_utils.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,13 +78,26 @@ class ParsedDocument:
text: str


def _directory_read_error(path: str) -> str:
return (
f"Error: `{path}` is a directory, not a file. "
"Use a file path instead, or use `astrbot_execute_shell` "
"to list directory contents."
)


def _build_probe_script(path: str) -> str:
directory_error = _directory_read_error(path)
return f"""
import base64
import json
from pathlib import Path

path = Path({path!r})
if path.is_dir():
raise ValueError({directory_error!r})
if not path.is_file():
raise ValueError("Error: File does not exist: `{path}`")
Comment on lines +99 to +100

@gemini-code-assist gemini-code-assist Bot Jun 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

If path contains double quotes (e.g., some"file.txt), interpolating it directly into the double-quoted string in the generated Python script will cause a SyntaxError at runtime. Instead, we should use an f-string in the generated script and evaluate the path variable (which is a Path object) at runtime.

Suggested change
if not path.is_file():
raise ValueError("Error: File does not exist: `{path}`")
if not path.is_file():
raise ValueError(f"Error: File does not exist: {path}")

Comment on lines +99 to +100

@sourcery-ai sourcery-ai Bot Jun 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

issue (bug_risk): The error message in the probe script will literally contain {path} instead of the actual path.

In the generated probe script this string isn’t an f-string, so {path} will appear literally in the error. To include the actual file path (as in the local branch), either make the generated script’s string an f-string or format the message in the outer Python code (e.g. raise ValueError(f"Error: File does not exist: {path}")) so the embedded script gets a fully formatted message.

with path.open("rb") as file_obj:
sample = file_obj.read({_FILE_SNIFF_BYTES})
print(
Expand Down Expand Up @@ -652,13 +665,24 @@ async def read_file_tool_result(
workspace_dir: str | None = None,
) -> ToolExecResult:
if local_mode:
file_path = Path(path)
if file_path.is_dir():
return _directory_read_error(path)
if not file_path.is_file():
return f"Error: File does not exist: `{path}`"
probe_payload = await _probe_local_file(path)
else:
probe_payload = await _exec_python_json(
booter,
_build_probe_script(path),
action="file probe",
)
try:
probe_payload = await _exec_python_json(
booter,
_build_probe_script(path),
action="file probe",
)
except RuntimeError as exc:
probe_error = str(exc).removeprefix("file probe failed: ").strip()
if probe_error.startswith("Error:"):
return probe_error
raise
Comment on lines +681 to +685

@gemini-code-assist gemini-code-assist Bot Jun 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

high

In remote/sandbox mode, when the probe script raises a ValueError, the python interpreter prints a traceback to stderr. This traceback is captured in probe_error, meaning it starts with 'Traceback (most recent call last):' rather than 'Error:'. As a result, probe_error.startswith("Error:") evaluates to False, and the RuntimeError is re-raised instead of returning the clean error message. We should parse the traceback lines to find the actual error message.

Suggested change
except RuntimeError as exc:
probe_error = str(exc).removeprefix("file probe failed: ").strip()
if probe_error.startswith("Error:"):
return probe_error
raise
except RuntimeError as exc:
probe_error = str(exc).removeprefix("file probe failed: ").strip()
for line in reversed(probe_error.splitlines()):
line_stripped = line.strip()
if line_stripped.startswith("ValueError: Error:"):
return line_stripped.removeprefix("ValueError: ").strip()
if line_stripped.startswith("Error:"):
return line_stripped
raise

sample_b64 = str(probe_payload.get("sample_b64", "") or "")
sample = base64.b64decode(sample_b64) if sample_b64 else b""
size_bytes = int(probe_payload.get("size_bytes", 0) or 0)
Expand Down
20 changes: 20 additions & 0 deletions tests/test_computer_fs_tools.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,6 +407,26 @@ def test_detect_text_encoding_allows_utf8_probe_cut_mid_character():
assert file_read_utils.detect_text_encoding(sample) in {"utf-8", "utf-8-sig"}


@pytest.mark.asyncio
async def test_file_read_tool_rejects_directory_path(
monkeypatch: pytest.MonkeyPatch,
tmp_path,
):
workspace = _setup_local_fs_tools(monkeypatch, tmp_path)
directory = workspace / "api"
directory.mkdir()

result = await fs_tools.FileReadTool().call(
_make_context(),
path="api",
)

assert "is a directory, not a file" in result
assert "astrbot_execute_shell" in result
assert "Permission denied" not in result
assert "Is a directory" not in result

Comment on lines +411 to +428

@gemini-code-assist gemini-code-assist Bot Jun 30, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

The current test only covers local_mode=True. Since the remote/sandbox mode uses a completely different code path (executing a Python probe script and parsing its stderr), we should add a unit test to verify that directory path rejection works correctly in sandbox mode as well.

@pytest.mark.asyncio
async def test_file_read_tool_rejects_directory_path(
    monkeypatch: pytest.MonkeyPatch,
    tmp_path,
):
    workspace = _setup_local_fs_tools(monkeypatch, tmp_path)
    directory = workspace / "api"
    directory.mkdir()

    result = await fs_tools.FileReadTool().call(
        _make_context(),
        path="api",
    )

    assert "is a directory, not a file" in result
    assert "astrbot_execute_shell" in result
    assert "Permission denied" not in result
    assert "Is a directory" not in result


@pytest.mark.asyncio
async def test_file_read_tool_rejects_directory_path_sandbox(
    monkeypatch: pytest.MonkeyPatch,
):
    booter = SimpleNamespace(
        python=SimpleNamespace(
            exec=AsyncMock(
                return_value={
                    "data": {
                        "output": {},
                        "error": (
                            "Traceback (most recent call last):\n"
                            "  File \"<string>\", line 8, in <module>\n"
                            "ValueError: Error: 'api' is a directory, not a file. "
                            "Use a file path instead, or use astrbot_execute_shell "
                            "to list directory contents."
                        )
                    }
                }
            )
        )
    )

    async def _fake_get_booter(_ctx, _umo):
        return booter

    monkeypatch.setattr(fs_tools, "get_booter", _fake_get_booter)

    result = await fs_tools.FileReadTool().call(
        _make_sandbox_context(),
        path="api",
    )

    assert "is a directory, not a file" in result
    assert "astrbot_execute_shell" in result
References
  1. New functionality, such as handling attachments, should be accompanied by corresponding unit tests.


@pytest.mark.asyncio
async def test_file_read_tool_rejects_large_full_text_read_before_local_stream_read(
monkeypatch: pytest.MonkeyPatch,
Expand Down