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

[THE-FLIER]

Summary

...

Summary by Sourcery

Clarify error handling when the file read tool is given invalid paths, especially directories, and cover the behavior with tests.

Bug Fixes:

• Return a clear, user-friendly error when the file read tool is invoked with a directory path instead of a file.
• Normalize error propagation from the remote probe script so structured file read errors are surfaced instead of generic runtime failures.

Tests:

• Add an async test to verify the file read tool rejects directory paths with the new, descriptive error message and without leaking low-level OS error text.

[gemini-code-assist]

Code Review

This pull request introduces checks to prevent reading directories as files in both local and remote modes, returning clear error messages instead of system errors. The review feedback highlights three key areas for improvement: resolving a potential syntax error in the generated probe script when paths contain double quotes, fixing the remote error-parsing logic which currently fails to extract the error message from tracebacks, and adding a unit test to cover directory rejection in sandbox/remote mode.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[gemini-code-assist]

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}")

[gemini-code-assist]

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

[gemini-code-assist]

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.

[sourcery-ai]

Hey - I've found 1 issue, and left some high level feedback:

• The remote and local code paths now each perform their own directory/file checks and construct error strings separately; consider centralizing this logic (and the "file does not exist" message) to avoid divergence in behavior and wording over time.
• The RuntimeError handling in the remote path relies on a hard-coded "file probe failed: " prefix; if that prefix changes in _exec_python_json this will silently break, so it may be worth tightening this contract (e.g., by using a structured error type or a dedicated flag in the JSON response) instead of parsing the message string.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The remote and local code paths now each perform their own directory/file checks and construct error strings separately; consider centralizing this logic (and the "file does not exist" message) to avoid divergence in behavior and wording over time.
- The `RuntimeError` handling in the remote path relies on a hard-coded `"file probe failed: "` prefix; if that prefix changes in `_exec_python_json` this will silently break, so it may be worth tightening this contract (e.g., by using a structured error type or a dedicated flag in the JSON response) instead of parsing the message string.

## Individual Comments

### Comment 1
<location path="astrbot/core/computer/file_read_utils.py" line_range="99-100" />
<code_context>
 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}`")
 with path.open("rb") as file_obj:
     sample = file_obj.read({_FILE_SNIFF_BYTES})
</code_context>
<issue_to_address>
**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.
</issue_to_address>

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[sourcery-ai]

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.

[THE-FLIER]

Closing in favor of #9088, which fixes the same issue with a smaller, focused change. Thanks for the review feedback!