review: tighten scope and add regression tests

Follow-ups from review of #3194:

- ci.yml: drop unconditional --ignore=tests/channels/test_matrix_channel.py.
  That test file already calls pytest.importorskip("nio") at module top, so
  it self-skips on Windows (where nio isn't installed) without also hiding
  62 tests from Linux CI.

- filesystem.py: hoist `import os` to the module top and drop the duplicate
  inline import in ReadFileTool.execute. Document the CRLF->LF normalization
  as intentional (primarily a Windows UX fix so downstream StrReplace/Grep
  match consistently regardless of where the file was written).

- test_read_enhancements.py: lock down two new behaviors
  * TestFileStateHashFallback: check_read warns when content changes but
    mtime is unchanged (coarse-mtime filesystems on Windows).
  * TestReadFileLineEndingNormalization: ReadFileTool strips CRLF and
    preserves LF-only files untouched.

- test_tool_validation.py: restore list2cmdline/shlex.quote in
  test_exec_head_tail_truncation. The temp_path-based form was correct,
  but dropping the quoting broke on any Windows path containing spaces
  (e.g. C:\Users\John Doe\...). CI runners happen not to have spaces so
  this slipped through.

Tests: 1993 passed locally.
Made-with: Cursor

.github/workflows/ci.yml

@@ -36,4 +36,4 @@ jobs:
       run: uv run ruff check nanobot --select F401,F841
 
     - name: Run tests
-      run: uv run pytest tests/ --ignore=tests/channels/test_matrix_channel.py
+      run: uv run pytest tests/

nanobot/agent/tools/filesystem.py

@@ -2,6 +2,7 @@
 
 import difflib
 import mimetypes
+import os
 from dataclasses import dataclass
 from pathlib import Path
 from typing import Any
@@ -178,7 +179,6 @@ class ReadFileTool(_FsTool):
 
             # Read dedup: same path + offset + limit + unchanged mtime → stub
             # Always check for external modifications before dedup
-            import os
             entry = file_state._state.get(str(fp.resolve()))
             try:
                 current_mtime = os.path.getmtime(fp)
@@ -218,6 +218,10 @@ class ReadFileTool(_FsTool):
                     return build_image_content_blocks(raw, mime, str(fp), f"(Image file: {path})")
                 return f"Error: Cannot read binary file {path} (MIME: {mime or 'unknown'}). Only UTF-8 text and images are supported."
 
+            # Normalize CRLF -> LF before line-splitting. Primarily a Windows
+            # concern (git checkouts with autocrlf, editors saving CRLF) but
+            # applied on all platforms so downstream StrReplace/Grep behavior
+            # is consistent regardless of where the file was written.
             text_content = text_content.replace("\r\n", "\n")
 
             all_lines = text_content.splitlines()

tests/tools/test_read_enhancements.py

@@ -1,5 +1,6 @@
 """Tests for ReadFileTool enhancements: description fix, read dedup, PDF support, device blacklist."""
 
+import os
 import sys
 
 import pytest
@@ -181,3 +182,67 @@ class TestReadDeviceBlacklist:
         result = await tool.execute(path=str(link))
         assert "Error" in result
         assert "blocked" in result.lower() or "device" in result.lower()
+
+
+# ---------------------------------------------------------------------------
+# file_state: mtime-unchanged / content-changed fallback
+# ---------------------------------------------------------------------------
+# On filesystems with coarse mtime resolution (NTFS ~100ms, FAT 2s) a fast
+# write-after-read can leave mtime unchanged. The content-hash fallback is
+# what protects against stale-read warnings being false-negative on those
+# platforms. Lock that behavior down here so nobody reverts it silently.
+
+class TestFileStateHashFallback:
+
+    def test_check_read_warns_when_content_changed_but_mtime_same(self, tmp_path):
+        f = tmp_path / "data.txt"
+        f.write_text("original", encoding="utf-8")
+        file_state.record_read(f)
+        original_mtime = os.path.getmtime(f)
+
+        f.write_text("modified", encoding="utf-8")
+        os.utime(f, (original_mtime, original_mtime))
+        assert os.path.getmtime(f) == original_mtime
+
+        warning = file_state.check_read(f)
+        assert warning is not None
+        assert "modified" in warning.lower()
+
+    def test_check_read_passes_when_content_and_mtime_unchanged(self, tmp_path):
+        f = tmp_path / "data.txt"
+        f.write_text("stable", encoding="utf-8")
+        file_state.record_read(f)
+
+        assert file_state.check_read(f) is None
+
+
+# ---------------------------------------------------------------------------
+# Line-ending normalization
+# ---------------------------------------------------------------------------
+# ReadFileTool normalizes CRLF -> LF before line-splitting. This primarily
+# helps Windows users whose checkouts carry CRLF line endings and whose
+# subsequent StrReplace edits would otherwise miss on `\r` boundaries. The
+# normalization applies on all platforms; these tests lock that in so the
+# behavior is intentional and discoverable, not accidental.
+
+class TestReadFileLineEndingNormalization:
+
+    @pytest.fixture()
+    def tool(self, tmp_path):
+        return ReadFileTool(workspace=tmp_path)
+
+    @pytest.mark.asyncio
+    async def test_crlf_is_normalized_to_lf(self, tool, tmp_path):
+        f = tmp_path / "crlf.txt"
+        f.write_bytes(b"alpha\r\nbeta\r\ngamma\r\n")
+        result = await tool.execute(path=str(f))
+        assert "\r" not in result
+        assert "alpha" in result and "beta" in result and "gamma" in result
+
+    @pytest.mark.asyncio
+    async def test_lf_only_is_preserved(self, tool, tmp_path):
+        f = tmp_path / "lf.txt"
+        f.write_bytes(b"alpha\nbeta\ngamma\n")
+        result = await tool.execute(path=str(f))
+        assert "\r" not in result
+        assert "alpha" in result and "beta" in result and "gamma" in result

tests/tools/test_tool_validation.py

@@ -1,3 +1,4 @@
+import shlex
 import subprocess
 import sys
 from typing import Any
@@ -547,13 +548,20 @@ async def test_exec_always_returns_exit_code() -> None:
 async def test_exec_head_tail_truncation(tmp_path) -> None:
     """Long output should preserve both head and tail."""
     tool = ExecTool()
-    # Generate output that exceeds _MAX_OUTPUT (10_000 chars)
-    # Use a temp script file to avoid Windows command line quote parsing issues
+    # Generate output that exceeds _MAX_OUTPUT (10_000 chars).
+    # Use a temp script file so the output-generating logic lives in a file
+    # (Windows cmd.exe has finicky rules for quoting `-c` payloads with
+    # embedded newlines). ExecTool runs via create_subprocess_shell, so we
+    # must quote *both* the interpreter path and the script path — tmp_path
+    # on some CI runners and on many local Windows installs contains spaces
+    # (e.g. C:\Users\John Doe\AppData\...) which would otherwise break the
+    # shell's argv split.
     script_file = tmp_path / "gen_output.py"
     script_file.write_text("print('A' * 6000 + chr(10) + 'B' * 6000)", encoding="utf-8")
-    # On Windows, cmd.exe handles quotes differently. Use the path directly
-    # without additional quotes since the temp path shouldn't have spaces.
-    command = f"{sys.executable} {script_file}"
+    if sys.platform == "win32":
+        command = subprocess.list2cmdline([sys.executable, str(script_file)])
+    else:
+        command = f"{shlex.quote(sys.executable)} {shlex.quote(str(script_file))}"
     result = await tool.execute(command=command)
     assert "chars truncated" in result
     # Head portion should start with As